patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size
       [not found] <b45c3416-0b1d-0ee4-89eb-c23a69e7cef3@intel.com>
@ 2019-11-12 15:18 ` Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
                     ` (2 more replies)
  2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:19 ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
  2 siblings, 3 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:18 UTC (permalink / raw)
  To: dev, stable; +Cc: Stefan Hajnoczi, Maxime Coquelin

From: Stefan Hajnoczi <stefanha@redhat.com>

[ backported from upstream commit eb7c574b21cc92792ea5a1f219ddf6dd3cf3b1e1 ]

Check the virtqueue size constraints so that invalid values don't cause
bugs later on in the code.  For example, sometimes the virtqueue size is
stored as unsigned int and sometimes as uint16_t, so bad things happen
if it is ever larger than 65535.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 618d413fe1..8a01c295e7 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -189,6 +189,17 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 
 	vq->size = state->num;
 
+	/* VIRTIO 1.0, 2.4 Virtqueues says:
+	 *
+	 *   Queue Size value is always a power of 2. The maximum Queue Size
+	 *   value is 32768.
+	 */
+	if ((vq->size & (vq->size - 1)) || vq->size > 32768) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"invalid virtqueue size %u\n", vq->size);
+		return -1;
+	}
+
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v16.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages
  2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
@ 2019-11-12 15:18   ` Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:18 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin, Dr . David Alan Gilbert

As soon as some ancillary data (fds) are received, it is copied
without checking its length.

This patch adds the number of fds received to the message,
which is set in read_vhost_message().

This is preliminary work to support sending fds to Qemu.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
(cherry picked from commit c00bb88d35fe975ede0ea35bdf4f765a2cece7e8)
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/socket.c     | 22 +++++++++++++++++-----
 lib/librte_vhost/vhost_user.c |  2 +-
 lib/librte_vhost/vhost_user.h |  4 +++-
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 805b2e5b23..4a19280fb3 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -101,17 +101,23 @@ static struct vhost_user vhost_user = {
 	.mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-/* return bytes# of read on success or negative val on failure. */
+/*
+ * return bytes# of read on success or negative val on failure. Update fdnum
+ * with number of fds read.
+ */
 int
-read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
+read_fd_message(int sockfd, char *buf, int buflen, int *fds, int max_fds,
+		int *fd_num)
 {
 	struct iovec iov;
 	struct msghdr msgh;
-	size_t fdsize = fd_num * sizeof(int);
-	char control[CMSG_SPACE(fdsize)];
+	char control[CMSG_SPACE(max_fds * sizeof(int))];
 	struct cmsghdr *cmsg;
+	int got_fds = 0;
 	int ret;
 
+	*fd_num = 0;
+
 	memset(&msgh, 0, sizeof(msgh));
 	iov.iov_base = buf;
 	iov.iov_len  = buflen;
@@ -136,11 +142,17 @@ read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
 		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
 		if ((cmsg->cmsg_level == SOL_SOCKET) &&
 			(cmsg->cmsg_type == SCM_RIGHTS)) {
-			memcpy(fds, CMSG_DATA(cmsg), fdsize);
+			got_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
+			*fd_num = got_fds;
+			memcpy(fds, CMSG_DATA(cmsg), got_fds * sizeof(int));
 			break;
 		}
 	}
 
+	/* Clear out unused file descriptors */
+	while (got_fds < max_fds)
+		fds[got_fds++] = -1;
+
 	return ret;
 }
 
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8a01c295e7..b8f6a9fba5 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -963,7 +963,7 @@ read_vhost_message(int sockfd, struct VhostUserMsg *msg)
 	int ret;
 
 	ret = read_fd_message(sockfd, (char *)msg, VHOST_USER_HDR_SIZE,
-		msg->fds, VHOST_MEMORY_MAX_NREGIONS);
+		msg->fds, VHOST_MEMORY_MAX_NREGIONS, &msg->fd_num);
 	if (ret <= 0)
 		return ret;
 
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index ba78d32684..ea9d304f82 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -110,6 +110,7 @@ typedef struct VhostUserMsg {
 		VhostUserLog    log;
 	} payload;
 	int fds[VHOST_MEMORY_MAX_NREGIONS];
+	int fd_num;
 } __attribute((packed)) VhostUserMsg;
 
 #define VHOST_USER_HDR_SIZE offsetof(VhostUserMsg, payload.u64)
@@ -122,7 +123,8 @@ typedef struct VhostUserMsg {
 int vhost_user_msg_handler(int vid, int fd);
 
 /* socket.c */
-int read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num);
+int read_fd_message(int sockfd, char *buf, int buflen, int *fds, int max_fds,
+		int *fd_num);
 int send_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num);
 
 #endif
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v16.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM
  2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
@ 2019-11-12 15:18   ` Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:18 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin, Jason Wang

vhost_user_set_vring_num() performs multiple allocations
without checking whether data were previously allocated.

It may cause a denial of service because of the memory leaks
that happen if a malicious vhost-user master keeps sending
VHOST_USER_SET_VRING_NUM request until the slave runs out
of memory.

This issue has been assigned CVE-2019-14818

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index b8f6a9fba5..cebc72f78f 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -204,6 +204,8 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
 		vq->zmbuf_size = vq->size;
+		if (vq->zmbufs)
+			rte_free(vq->zmbufs);
 		vq->zmbufs = rte_zmalloc(NULL, vq->zmbuf_size *
 					 sizeof(struct zcopy_mbuf), 0);
 		if (vq->zmbufs == NULL) {
@@ -213,7 +215,8 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 			dev->dequeue_zero_copy = 0;
 		}
 	}
-
+	if (vq->shadow_used_ring)
+		rte_free(vq->shadow_used_ring);
 	vq->shadow_used_ring = rte_malloc(NULL,
 				vq->size * sizeof(struct vring_used_elem),
 				RTE_CACHE_LINE_SIZE);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v16.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs
  2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
  2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
@ 2019-11-12 15:18   ` Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:18 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin

A malicious Vhost-user master could send in loop hand-crafted
vhost-user messages containing more file descriptors the
vhost-user slave expects. Doing so causes the application using
the vhost-user library to run out of FDs.

This issue has been assigned CVE-2019-14818

Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 87 +++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index cebc72f78f..a6ab131543 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -75,6 +75,36 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_SEND_RARP]  = "VHOST_USER_SEND_RARP",
 };
 
+static void
+close_msg_fds(struct VhostUserMsg *msg)
+{
+	int i;
+
+	for (i = 0; i < msg->fd_num; i++)
+		close(msg->fds[i]);
+}
+
+/*
+ * Ensure the expected number of FDs is received,
+ * close all FDs and return an error if this is not the case.
+ */
+static int
+validate_msg_fds(struct VhostUserMsg *msg, int expected_fds)
+{
+	if (msg->fd_num == expected_fds)
+		return 0;
+
+	RTE_LOG(ERR, VHOST_CONFIG,
+		" Expect %d FDs for request %s, received %d\n",
+		expected_fds,
+		vhost_message_str[msg->request],
+		msg->fd_num);
+
+	close_msg_fds(msg);
+
+	return -1;
+}
+
 static uint64_t
 get_blk_size(int fd)
 {
@@ -1104,35 +1134,59 @@ vhost_user_msg_handler(int vid, int fd)
 	ret = 0;
 	switch (msg.request) {
 	case VHOST_USER_GET_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		msg.payload.u64 = vhost_user_get_features();
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_message(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		msg.payload.u64 = VHOST_USER_PROTOCOL_FEATURES;
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_message(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_protocol_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_SET_OWNER:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_owner();
 		break;
 	case VHOST_USER_RESET_OWNER:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_reset_owner(dev);
 		break;
 
 	case VHOST_USER_SET_MEM_TABLE:
+		if (validate_msg_fds(&msg, msg.payload.memory.nregions) != 0)
+			return -1;
+
 		vhost_user_set_mem_table(dev, &msg);
 		break;
 
 	case VHOST_USER_SET_LOG_BASE:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		ret = vhost_user_set_log_base(dev, &msg);
 		if (ret)
 			break;
@@ -1144,21 +1198,36 @@ vhost_user_msg_handler(int vid, int fd)
 		send_vhost_message(fd, &msg);
 		break;
 	case VHOST_USER_SET_LOG_FD:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		close(msg.fds[0]);
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_vring_num(dev, &msg.payload.state);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_vring_addr(&dev, &msg.payload.addr);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_vring_base(dev, &msg.payload.state);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_get_vring_base(dev, &msg.payload.state);
 		if (ret)
 			break;
@@ -1167,28 +1236,46 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_KICK:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		ret = vhost_user_set_vring_kick(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_CALL:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		vhost_user_set_vring_call(dev, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ERR:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		if (!(msg.payload.u64 & VHOST_USER_VRING_NOFD_MASK))
 			close(msg.fds[0]);
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
 		break;
 
 	case VHOST_USER_GET_QUEUE_NUM:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		msg.payload.u64 = VHOST_MAX_QUEUE_PAIRS;
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_message(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_set_vring_enable(dev, &msg.payload.state);
 		break;
 	case VHOST_USER_SEND_RARP:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_send_rarp(dev, &msg);
 		break;
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size
       [not found] <b45c3416-0b1d-0ee4-89eb-c23a69e7cef3@intel.com>
  2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
@ 2019-11-12 15:19 ` Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
                     ` (2 more replies)
  2019-11-12 15:19 ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
  2 siblings, 3 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Stefan Hajnoczi, Maxime Coquelin

From: Stefan Hajnoczi <stefanha@redhat.com>

[ backported from upstream commit eb7c574b21cc92792ea5a1f219ddf6dd3cf3b1e1 ]

Check the virtqueue size constraints so that invalid values don't cause
bugs later on in the code.  For example, sometimes the virtqueue size is
stored as unsigned int and sometimes as uint16_t, so bad things happen
if it is ever larger than 65535.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index bb39999aa4..93e871c5bb 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -247,6 +247,17 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 
 	vq->size = msg->payload.state.num;
 
+	/* VIRTIO 1.0, 2.4 Virtqueues says:
+	 *
+	 *   Queue Size value is always a power of 2. The maximum Queue Size
+	 *   value is 32768.
+	 */
+	if ((vq->size & (vq->size - 1)) || vq->size > 32768) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"invalid virtqueue size %u\n", vq->size);
+		return -1;
+	}
+
 	if (dev->dequeue_zero_copy) {
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v17.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages
  2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
@ 2019-11-12 15:19   ` Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin, Dr . David Alan Gilbert

As soon as some ancillary data (fds) are received, it is copied
without checking its length.

This patch adds the number of fds received to the message,
which is set in read_vhost_message().

This is preliminary work to support sending fds to Qemu.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
(cherry picked from commit c00bb88d35fe975ede0ea35bdf4f765a2cece7e8)
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/socket.c     | 22 +++++++++++++++++-----
 lib/librte_vhost/vhost_user.c |  2 +-
 lib/librte_vhost/vhost_user.h |  4 +++-
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 88be697c2f..2fa7ea0e09 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -117,17 +117,23 @@ static struct vhost_user vhost_user = {
 	.mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-/* return bytes# of read on success or negative val on failure. */
+/*
+ * return bytes# of read on success or negative val on failure. Update fdnum
+ * with number of fds read.
+ */
 int
-read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
+read_fd_message(int sockfd, char *buf, int buflen, int *fds, int max_fds,
+		int *fd_num)
 {
 	struct iovec iov;
 	struct msghdr msgh;
-	size_t fdsize = fd_num * sizeof(int);
-	char control[CMSG_SPACE(fdsize)];
+	char control[CMSG_SPACE(max_fds * sizeof(int))];
 	struct cmsghdr *cmsg;
+	int got_fds = 0;
 	int ret;
 
+	*fd_num = 0;
+
 	memset(&msgh, 0, sizeof(msgh));
 	iov.iov_base = buf;
 	iov.iov_len  = buflen;
@@ -152,11 +158,17 @@ read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num)
 		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
 		if ((cmsg->cmsg_level == SOL_SOCKET) &&
 			(cmsg->cmsg_type == SCM_RIGHTS)) {
-			memcpy(fds, CMSG_DATA(cmsg), fdsize);
+			got_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
+			*fd_num = got_fds;
+			memcpy(fds, CMSG_DATA(cmsg), got_fds * sizeof(int));
 			break;
 		}
 	}
 
+	/* Clear out unused file descriptors */
+	while (got_fds < max_fds)
+		fds[got_fds++] = -1;
+
 	return ret;
 }
 
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 93e871c5bb..9773691097 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1248,7 +1248,7 @@ read_vhost_message(int sockfd, struct VhostUserMsg *msg)
 	int ret;
 
 	ret = read_fd_message(sockfd, (char *)msg, VHOST_USER_HDR_SIZE,
-		msg->fds, VHOST_MEMORY_MAX_NREGIONS);
+		msg->fds, VHOST_MEMORY_MAX_NREGIONS, &msg->fd_num);
 	if (ret <= 0)
 		return ret;
 
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index 76d9fe2fc5..10b8cc5e1a 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -130,6 +130,7 @@ typedef struct VhostUserMsg {
 		struct vhost_iotlb_msg iotlb;
 	} payload;
 	int fds[VHOST_MEMORY_MAX_NREGIONS];
+	int fd_num;
 } __attribute((packed)) VhostUserMsg;
 
 #define VHOST_USER_HDR_SIZE offsetof(VhostUserMsg, payload.u64)
@@ -143,7 +144,8 @@ int vhost_user_msg_handler(int vid, int fd);
 int vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm);
 
 /* socket.c */
-int read_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num);
+int read_fd_message(int sockfd, char *buf, int buflen, int *fds, int max_fds,
+		int *fd_num);
 int send_fd_message(int sockfd, char *buf, int buflen, int *fds, int fd_num);
 
 #endif
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v17.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM
  2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
@ 2019-11-12 15:19   ` Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin, Jason Wang

vhost_user_set_vring_num() performs multiple allocations
without checking whether data were previously allocated.

It may cause a denial of service because of the memory leaks
that happen if a malicious vhost-user master keeps sending
VHOST_USER_SET_VRING_NUM request until the slave runs out
of memory.

This issue has been assigned CVE-2019-14818

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 9773691097..781734e9e3 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -262,6 +262,8 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
 		vq->zmbuf_size = vq->size;
+		if (vq->zmbufs)
+			rte_free(vq->zmbufs);
 		vq->zmbufs = rte_zmalloc(NULL, vq->zmbuf_size *
 					 sizeof(struct zcopy_mbuf), 0);
 		if (vq->zmbufs == NULL) {
@@ -271,7 +273,8 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 			dev->dequeue_zero_copy = 0;
 		}
 	}
-
+	if (vq->shadow_used_ring)
+		rte_free(vq->shadow_used_ring);
 	vq->shadow_used_ring = rte_malloc(NULL,
 				vq->size * sizeof(struct vring_used_elem),
 				RTE_CACHE_LINE_SIZE);
@@ -281,6 +284,8 @@ vhost_user_set_vring_num(struct virtio_net *dev,
 		return -1;
 	}
 
+	if (vq->batch_copy_elems)
+		rte_free(vq->batch_copy_elems);
 	vq->batch_copy_elems = rte_malloc(NULL,
 				vq->size * sizeof(struct batch_copy_elem),
 				RTE_CACHE_LINE_SIZE);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v17.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs
  2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
@ 2019-11-12 15:19   ` Maxime Coquelin
  2 siblings, 0 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin

A malicious Vhost-user master could send in loop hand-crafted
vhost-user messages containing more file descriptors the
vhost-user slave expects. Doing so causes the application using
the vhost-user library to run out of FDs.

This issue has been assigned CVE-2019-14818

Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 95 +++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 781734e9e3..d4643dc350 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -81,6 +81,36 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_IOTLB_MSG]  = "VHOST_USER_IOTLB_MSG",
 };
 
+static void
+close_msg_fds(struct VhostUserMsg *msg)
+{
+	int i;
+
+	for (i = 0; i < msg->fd_num; i++)
+		close(msg->fds[i]);
+}
+
+/*
+ * Ensure the expected number of FDs is received,
+ * close all FDs and return an error if this is not the case.
+ */
+static int
+validate_msg_fds(struct VhostUserMsg *msg, int expected_fds)
+{
+	if (msg->fd_num == expected_fds)
+		return 0;
+
+	RTE_LOG(ERR, VHOST_CONFIG,
+		" Expect %d FDs for request %s, received %d\n",
+		expected_fds,
+		vhost_message_str[msg->request.master],
+		msg->fd_num);
+
+	close_msg_fds(msg);
+
+	return -1;
+}
+
 static uint64_t
 get_blk_size(int fd)
 {
@@ -1458,34 +1488,58 @@ vhost_user_msg_handler(int vid, int fd)
 
 	switch (msg.request.master) {
 	case VHOST_USER_GET_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		msg.payload.u64 = vhost_user_get_features(dev);
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_get_protocol_features(dev, &msg);
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_protocol_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_SET_OWNER:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_owner();
 		break;
 	case VHOST_USER_RESET_OWNER:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_reset_owner(dev);
 		break;
 
 	case VHOST_USER_SET_MEM_TABLE:
+		if (validate_msg_fds(&msg, msg.payload.memory.nregions) != 0)
+			return -1;
+
 		ret = vhost_user_set_mem_table(&dev, &msg);
 		break;
 
 	case VHOST_USER_SET_LOG_BASE:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		vhost_user_set_log_base(dev, &msg);
 
 		/*
@@ -1496,61 +1550,102 @@ vhost_user_msg_handler(int vid, int fd)
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_LOG_FD:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		close(msg.fds[0]);
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_vring_addr(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_get_vring_base(dev, &msg);
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_reply(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_KICK:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		vhost_user_set_vring_kick(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_CALL:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		vhost_user_set_vring_call(dev, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ERR:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		if (!(msg.payload.u64 & VHOST_USER_VRING_NOFD_MASK))
 			close(msg.fds[0]);
 		RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
 		break;
 
 	case VHOST_USER_GET_QUEUE_NUM:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
 		msg.payload.u64 = VHOST_MAX_QUEUE_PAIRS;
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_reply(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		vhost_user_send_rarp(dev, &msg);
 		break;
 
 	case VHOST_USER_NET_SET_MTU:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_net_set_mtu(dev, &msg);
 		break;
 
 	case VHOST_USER_SET_SLAVE_REQ_FD:
+		if (validate_msg_fds(&msg, 1) != 0)
+			return -1;
+
 		ret = vhost_user_set_req_fd(dev, &msg);
 		break;
 
 	case VHOST_USER_IOTLB_MSG:
+		if (validate_msg_fds(&msg, 0) != 0)
+			return -1;
+
 		ret = vhost_user_iotlb_msg(&dev, &msg);
 		break;
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM
       [not found] <b45c3416-0b1d-0ee4-89eb-c23a69e7cef3@intel.com>
  2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
  2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
@ 2019-11-12 15:19 ` Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
  2019-11-12 15:29   ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Kevin Traynor
  2 siblings, 2 replies; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin, Jason Wang

vhost_user_set_vring_num() performs multiple allocations
without checking whether data were previously allocated.

It may cause a denial of service because of the memory leaks
that happen if a malicious vhost-user master keeps sending
VHOST_USER_SET_VRING_NUM request until the slave runs out
of memory.

This issue has been assigned CVE-2019-14818

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5552f8bbfb..457e62d97e 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -346,6 +346,8 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 		vq->nr_zmbuf = 0;
 		vq->last_zmbuf_idx = 0;
 		vq->zmbuf_size = vq->size;
+		if (vq->zmbufs)
+			rte_free(vq->zmbufs);
 		vq->zmbufs = rte_zmalloc(NULL, vq->zmbuf_size *
 					 sizeof(struct zcopy_mbuf), 0);
 		if (vq->zmbufs == NULL) {
@@ -358,6 +360,8 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 	}
 
 	if (vq_is_packed(dev)) {
+		if (vq->shadow_used_packed)
+			rte_free(vq->shadow_used_packed);
 		vq->shadow_used_packed = rte_malloc(NULL,
 				vq->size *
 				sizeof(struct vring_used_elem_packed),
@@ -369,6 +373,8 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 		}
 
 	} else {
+		if (vq->shadow_used_split)
+			rte_free(vq->shadow_used_split);
 		vq->shadow_used_split = rte_malloc(NULL,
 				vq->size * sizeof(struct vring_used_elem),
 				RTE_CACHE_LINE_SIZE);
@@ -379,6 +385,8 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 		}
 	}
 
+	if (vq->batch_copy_elems)
+		rte_free(vq->batch_copy_elems);
 	vq->batch_copy_elems = rte_malloc(NULL,
 				vq->size * sizeof(struct batch_copy_elem),
 				RTE_CACHE_LINE_SIZE);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs
  2019-11-12 15:19 ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
@ 2019-11-12 15:19   ` Maxime Coquelin
  2019-11-12 15:29     ` Kevin Traynor
  2019-11-12 15:29   ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Kevin Traynor
  1 sibling, 1 reply; 12+ messages in thread
From: Maxime Coquelin @ 2019-11-12 15:19 UTC (permalink / raw)
  To: dev, stable; +Cc: Maxime Coquelin

A malicious Vhost-user master could send in loop hand-crafted
vhost-user messages containing more file descriptors the
vhost-user slave expects. Doing so causes the application using
the vhost-user library to run out of FDs.

This issue has been assigned CVE-2019-14818

Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 118 ++++++++++++++++++++++++++++++++--
 1 file changed, 114 insertions(+), 4 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 457e62d97e..98cd670e03 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -83,6 +83,36 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg);
 static int read_vhost_message(int sockfd, struct VhostUserMsg *msg);
 
+static void
+close_msg_fds(struct VhostUserMsg *msg)
+{
+	int i;
+
+	for (i = 0; i < msg->fd_num; i++)
+		close(msg->fds[i]);
+}
+
+/*
+ * Ensure the expected number of FDs is received,
+ * close all FDs and return an error if this is not the case.
+ */
+static int
+validate_msg_fds(struct VhostUserMsg *msg, int expected_fds)
+{
+	if (msg->fd_num == expected_fds)
+		return 0;
+
+	RTE_LOG(ERR, VHOST_CONFIG,
+		" Expect %d FDs for request %s, received %d\n",
+		expected_fds,
+		vhost_message_str[msg->request.master],
+		msg->fd_num);
+
+	close_msg_fds(msg);
+
+	return -1;
+}
+
 static uint64_t
 get_blk_size(int fd)
 {
@@ -179,18 +209,25 @@ vhost_backend_cleanup(struct virtio_net *dev)
  */
 static int
 vhost_user_set_owner(struct virtio_net **pdev __rte_unused,
-			struct VhostUserMsg *msg __rte_unused,
+			struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	return VH_RESULT_OK;
 }
 
 static int
 vhost_user_reset_owner(struct virtio_net **pdev,
-			struct VhostUserMsg *msg __rte_unused,
+			struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
 	struct virtio_net *dev = *pdev;
+
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	vhost_destroy_device_notify(dev);
 
 	cleanup_device(dev, 0);
@@ -208,6 +245,9 @@ vhost_user_get_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct virtio_net *dev = *pdev;
 	uint64_t features = 0;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	rte_vhost_driver_get_features(dev->ifname, &features);
 
 	msg->payload.u64 = features;
@@ -227,6 +267,9 @@ vhost_user_get_queue_num(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct virtio_net *dev = *pdev;
 	uint32_t queue_num = 0;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	rte_vhost_driver_get_queue_num(dev->ifname, &queue_num);
 
 	msg->payload.u64 = (uint64_t)queue_num;
@@ -249,6 +292,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	rte_vhost_driver_get_features(dev->ifname, &vhost_features);
 	if (features & ~vhost_features) {
 		RTE_LOG(ERR, VHOST_CONFIG,
@@ -329,6 +375,9 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	vq->size = msg->payload.state.num;
 
 	/* VIRTIO 1.0, 2.4 Virtqueues says:
@@ -708,6 +757,9 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct vhost_virtqueue *vq;
 	struct vhost_vring_addr *addr = &msg->payload.addr;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	if (dev->mem == NULL)
 		return VH_RESULT_ERR;
 
@@ -746,6 +798,9 @@ vhost_user_set_vring_base(struct virtio_net **pdev,
 	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 	uint64_t val = msg->payload.state.num;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	if (vq_is_packed(dev)) {
 		/*
 		 * Bit[0:14]: avail index
@@ -907,6 +962,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	int populate;
 	int fd;
 
+	if (validate_msg_fds(msg, memory->nregions) != 0)
+		return VH_RESULT_ERR;
+
 	if (memory->nregions > VHOST_MEMORY_MAX_NREGIONS) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"too many memory regions (%u)\n", memory->nregions);
@@ -917,8 +975,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"(%d) memory regions not changed\n", dev->vid);
 
-		for (i = 0; i < memory->nregions; i++)
-			close(msg->fds[i]);
+		close_msg_fds(msg);
 
 		return VH_RESULT_OK;
 	}
@@ -1061,6 +1118,10 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 				"Failed to read qemu ack on postcopy set-mem-table\n");
 			goto err_mmap;
 		}
+
+		if (validate_msg_fds(&ack_msg, 0) != 0)
+			goto err_mmap;
+
 		if (ack_msg.request.master != VHOST_USER_SET_MEM_TABLE) {
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"Bad qemu ack on postcopy set-mem-table (%d)\n",
@@ -1181,6 +1242,9 @@ vhost_user_set_vring_call(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
 
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
 	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
 		file.fd = VIRTIO_INVALID_EVENTFD;
@@ -1202,6 +1266,9 @@ static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 			struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	if (!(msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK))
 		close(msg->fds[0]);
 	RTE_LOG(INFO, VHOST_CONFIG, "not implemented\n");
@@ -1217,6 +1284,9 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
 
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
 	if (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
 		file.fd = VIRTIO_INVALID_EVENTFD;
@@ -1273,6 +1343,9 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	struct vhost_virtqueue *vq = dev->virtqueue[msg->payload.state.index];
 	uint64_t val;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	/* We have to stop the queue (virtio) if it is running. */
 	vhost_destroy_device_notify(dev);
 
@@ -1346,6 +1419,9 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	RTE_LOG(INFO, VHOST_CONFIG,
 		"set queue enable: %d to qp idx: %d\n",
 		enable, index);
@@ -1376,6 +1452,9 @@ vhost_user_get_protocol_features(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	uint64_t features, protocol_features;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	rte_vhost_driver_get_features(dev->ifname, &features);
 	rte_vhost_driver_get_protocol_features(dev->ifname, &protocol_features);
 
@@ -1404,6 +1483,9 @@ vhost_user_set_protocol_features(struct virtio_net **pdev,
 	uint64_t protocol_features = msg->payload.u64;
 	uint64_t slave_protocol_features = 0;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	rte_vhost_driver_get_protocol_features(dev->ifname,
 			&slave_protocol_features);
 	if (protocol_features & ~slave_protocol_features) {
@@ -1427,6 +1509,9 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	uint64_t size, off;
 	void *addr;
 
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	if (fd < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG, "invalid log fd: %d\n", fd);
 		return VH_RESULT_ERR;
@@ -1490,6 +1575,9 @@ static int vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused,
 			struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	close(msg->fds[0]);
 	RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n");
 
@@ -1513,6 +1601,9 @@ vhost_user_send_rarp(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct rte_vdpa_device *vdpa_dev;
 	int did = -1;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	RTE_LOG(DEBUG, VHOST_CONFIG,
 		":: mac: %02x:%02x:%02x:%02x:%02x:%02x\n",
 		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
@@ -1540,6 +1631,10 @@ vhost_user_net_set_mtu(struct virtio_net **pdev, struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
 	struct virtio_net *dev = *pdev;
+
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	if (msg->payload.u64 < VIRTIO_MIN_MTU ||
 			msg->payload.u64 > VIRTIO_MAX_MTU) {
 		RTE_LOG(ERR, VHOST_CONFIG, "Invalid MTU size (%"PRIu64")\n",
@@ -1560,6 +1655,9 @@ vhost_user_set_req_fd(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct virtio_net *dev = *pdev;
 	int fd = msg->fds[0];
 
+	if (validate_msg_fds(msg, 1) != 0)
+		return VH_RESULT_ERR;
+
 	if (fd < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 				"Invalid file descriptor for slave channel (%d)\n",
@@ -1630,6 +1728,9 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	uint16_t i;
 	uint64_t vva, len;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	switch (imsg->type) {
 	case VHOST_IOTLB_UPDATE:
 		len = imsg->size;
@@ -1676,6 +1777,9 @@ vhost_user_set_postcopy_advise(struct virtio_net **pdev,
 #ifdef RTE_LIBRTE_VHOST_POSTCOPY
 	struct uffdio_api api_struct;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	dev->postcopy_ufd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
 
 	if (dev->postcopy_ufd == -1) {
@@ -1711,6 +1815,9 @@ vhost_user_set_postcopy_listen(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	if (dev->mem && dev->mem->nregions) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"Regions already registered at postcopy-listen\n");
@@ -1727,6 +1834,9 @@ vhost_user_postcopy_end(struct virtio_net **pdev, struct VhostUserMsg *msg,
 {
 	struct virtio_net *dev = *pdev;
 
+	if (validate_msg_fds(msg, 0) != 0)
+		return VH_RESULT_ERR;
+
 	dev->postcopy_listening = 0;
 	if (dev->postcopy_ufd >= 0) {
 		close(dev->postcopy_ufd);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM
  2019-11-12 15:19 ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
  2019-11-12 15:19   ` [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
@ 2019-11-12 15:29   ` Kevin Traynor
  1 sibling, 0 replies; 12+ messages in thread
From: Kevin Traynor @ 2019-11-12 15:29 UTC (permalink / raw)
  To: Maxime Coquelin, dev, stable; +Cc: Jason Wang

On 12/11/2019 15:19, Maxime Coquelin wrote:
> vhost_user_set_vring_num() performs multiple allocations
> without checking whether data were previously allocated.
> 
> It may cause a denial of service because of the memory leaks
> that happen if a malicious vhost-user master keeps sending
> VHOST_USER_SET_VRING_NUM request until the slave runs out
> of memory.
> 
> This issue has been assigned CVE-2019-14818
> 
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Applied, thanks.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs
  2019-11-12 15:19   ` [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
@ 2019-11-12 15:29     ` Kevin Traynor
  0 siblings, 0 replies; 12+ messages in thread
From: Kevin Traynor @ 2019-11-12 15:29 UTC (permalink / raw)
  To: Maxime Coquelin, dev, stable

On 12/11/2019 15:19, Maxime Coquelin wrote:
> A malicious Vhost-user master could send in loop hand-crafted
> vhost-user messages containing more file descriptors the
> vhost-user slave expects. Doing so causes the application using
> the vhost-user library to run out of FDs.
> 
> This issue has been assigned CVE-2019-14818
> 
> Fixes: 8f972312b8f4 ("vhost: support vhost-user")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Applied, thanks.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-11-12 15:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <b45c3416-0b1d-0ee4-89eb-c23a69e7cef3@intel.com>
2019-11-12 15:18 ` [dpdk-stable] [v16.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
2019-11-12 15:18   ` [dpdk-stable] [v16.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
2019-11-12 15:19 ` [dpdk-stable] [v17.11 PATCH v2 1/4] vhost: validate virtqueue size Maxime Coquelin
2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 2/4] vhost: add number of fds to vhost-user messages Maxime Coquelin
2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 3/4] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
2019-11-12 15:19   ` [dpdk-stable] [v17.11 PATCH v2 4/4] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
2019-11-12 15:19 ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Maxime Coquelin
2019-11-12 15:19   ` [dpdk-stable] [v18.11 PATCH v2 2/2] vhost: fix possible denial of service by leaking FDs Maxime Coquelin
2019-11-12 15:29     ` Kevin Traynor
2019-11-12 15:29   ` [dpdk-stable] [v18.11 PATCH v2 1/2] vhost: fix possible denial of service on SET_VRING_NUM Kevin Traynor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).