DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/6] Fix vhost security issues
@ 2020-05-18 13:16 Ferruh Yigit
  2020-05-18 13:16 ` [dpdk-dev] [PATCH 1/6] vhost: check log mmap offset and size overflow Ferruh Yigit
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:16 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Maxime Coquelin

From: Maxime Coquelin <maxime.coquelin@redhat.com>

Marvin Liu (1):
  vhost: fix translated address not checked

Maxime Coquelin (3):
  vhost: check log mmap offset and size overflow
  vhost: fix vring index check
  vhost/crypto: validate keys lengths

Xiaolong Ye (1):
  vhost: fix potential memory space leak

Xuan Ding (1):
  vhost: fix potential fd leak

 lib/librte_vhost/vhost_crypto.c | 17 +++++++++++++++++
 lib/librte_vhost/vhost_user.c   | 30 +++++++++++++++++++++++-------
 lib/librte_vhost/virtio_net.c   |  4 ++++
 3 files changed, 44 insertions(+), 7 deletions(-)

-- 
2.25.2


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

* [dpdk-dev] [PATCH 1/6] vhost: check log mmap offset and size overflow
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
@ 2020-05-18 13:16 ` Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 2/6] vhost: fix vring index check Ferruh Yigit
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:16 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Maxime Coquelin, stable, Ilja Van Sprundel, Xiaolong Ye

From: Maxime Coquelin <maxime.coquelin@redhat.com>

vhost_user_set_log_base() is a message handler that is
called to handle the VHOST_USER_SET_LOG_BASE message.
Its payload contains a 64 bit size and offset. Both are
added up and used as a size when calling mmap().

There is no integer overflow check. If an integer overflow
occurs a smaller memory map would be created than
requested. Since the returned mapping is mapped as writable
and used for logging, a memory corruption could occur.

CVE-2020-10722
Fixes: fbc4d248b198 ("vhost: fix offset while mmaping log base address")
Cc: stable@dpdk.org

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
---
 lib/librte_vhost/vhost_user.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index bd1be01040..1eea371fc8 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2059,10 +2059,10 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	size = msg->payload.log.mmap_size;
 	off  = msg->payload.log.mmap_offset;
 
-	/* Don't allow mmap_offset to point outside the mmap region */
-	if (off > size) {
+	/* Check for mmap size and offset overflow. */
+	if (off >= -size) {
 		VHOST_LOG_CONFIG(ERR,
-			"log offset %#"PRIx64" exceeds log size %#"PRIx64"\n",
+			"log offset %#"PRIx64" and log size %#"PRIx64" overflow\n",
 			off, size);
 		return RTE_VHOST_MSG_RESULT_ERR;
 	}
-- 
2.25.2


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

* [dpdk-dev] [PATCH 2/6] vhost: fix vring index check
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
  2020-05-18 13:16 ` [dpdk-dev] [PATCH 1/6] vhost: check log mmap offset and size overflow Ferruh Yigit
@ 2020-05-18 13:17 ` Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 3/6] vhost/crypto: validate keys lengths Ferruh Yigit
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:17 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Maxime Coquelin, stable, Ilja Van Sprundel, Xiaolong Ye

From: Maxime Coquelin <maxime.coquelin@redhat.com>

vhost_user_check_and_alloc_queue_pair() is used to extract
a vring index from a payload. This function validates the
index and is called early on in when performing message
handling. Most message handlers depend on it correctly
validating the vring index.

Depending on the message type the vring index is in
different parts of the payload. The function contains a
switch/case for each type and copies the index. This is
stored in a uint16. This index is then validated. Depending
on the message, the source index is an unsigned int. If
integer truncation occurs (uint->uint16) the top 16 bits
of the index are never validated.

When they are used later on  (e.g. in
vhost_user_set_vring_num() or vhost_user_set_vring_addr())
it can lead to out of bound indexing. The out of bound
indexed data gets written to, and hence this can cause
memory corruption.

This patch fixes this vulnerability by declaring vring
index as an unsigned int in
vhost_user_check_and_alloc_queue_pair().

CVE-2020-10723
Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
Cc: stable@dpdk.org

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
---
 lib/librte_vhost/vhost_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1eea371fc8..e51a8a6b77 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2526,7 +2526,7 @@ static int
 vhost_user_check_and_alloc_queue_pair(struct virtio_net *dev,
 			struct VhostUserMsg *msg)
 {
-	uint16_t vring_idx;
+	uint32_t vring_idx;
 
 	switch (msg->request.master) {
 	case VHOST_USER_SET_VRING_KICK:
-- 
2.25.2


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

* [dpdk-dev] [PATCH 3/6] vhost/crypto: validate keys lengths
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
  2020-05-18 13:16 ` [dpdk-dev] [PATCH 1/6] vhost: check log mmap offset and size overflow Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 2/6] vhost: fix vring index check Ferruh Yigit
@ 2020-05-18 13:17 ` Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 4/6] vhost: fix translated address not checked Ferruh Yigit
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:17 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Maxime Coquelin, stable, Ilja Van Sprundel, Xiaolong Ye

From: Maxime Coquelin <maxime.coquelin@redhat.com>

transform_cipher_param() and transform_chain_param() handle
the payload data for the VHOST_USER_CRYPTO_CREATE_SESS
message. These payloads have to be validated, since it
could come from untrusted sources.

Two buffers and their lenghts are defined in this payload,
one the the auth key and one for the cipher key. But above
functions do not validate the key length inputs, which could
lead to read out of bounds, as buffers have static sizes of
64 bytes for the cipher key and 512 bytes for the auth key.

This patch adds necessary checks on the key length field
before being used.

CVE-2020-10724
Fixes: e80a98708166 ("vhost/crypto: add session message handler")
Cc: stable@dpdk.org

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
---
 lib/librte_vhost/vhost_crypto.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
index 2e52ecae87..0f9df4059d 100644
--- a/lib/librte_vhost/vhost_crypto.c
+++ b/lib/librte_vhost/vhost_crypto.c
@@ -238,6 +238,11 @@ transform_cipher_param(struct rte_crypto_sym_xform *xform,
 	if (unlikely(ret < 0))
 		return ret;
 
+	if (param->cipher_key_len > VHOST_USER_CRYPTO_MAX_CIPHER_KEY_LENGTH) {
+		VC_LOG_DBG("Invalid cipher key length\n");
+		return -VIRTIO_CRYPTO_BADMSG;
+	}
+
 	xform->type = RTE_CRYPTO_SYM_XFORM_CIPHER;
 	xform->cipher.key.length = param->cipher_key_len;
 	if (xform->cipher.key.length > 0)
@@ -288,6 +293,12 @@ transform_chain_param(struct rte_crypto_sym_xform *xforms,
 			&xform_cipher->cipher.algo);
 	if (unlikely(ret < 0))
 		return ret;
+
+	if (param->cipher_key_len > VHOST_USER_CRYPTO_MAX_CIPHER_KEY_LENGTH) {
+		VC_LOG_DBG("Invalid cipher key length\n");
+		return -VIRTIO_CRYPTO_BADMSG;
+	}
+
 	xform_cipher->type = RTE_CRYPTO_SYM_XFORM_CIPHER;
 	xform_cipher->cipher.key.length = param->cipher_key_len;
 	xform_cipher->cipher.key.data = param->cipher_key_buf;
@@ -302,6 +313,12 @@ transform_chain_param(struct rte_crypto_sym_xform *xforms,
 	ret = auth_algo_transform(param->hash_algo, &xform_auth->auth.algo);
 	if (unlikely(ret < 0))
 		return ret;
+
+	if (param->auth_key_len > VHOST_USER_CRYPTO_MAX_HMAC_KEY_LENGTH) {
+		VC_LOG_DBG("Invalid auth key length\n");
+		return -VIRTIO_CRYPTO_BADMSG;
+	}
+
 	xform_auth->auth.digest_length = param->digest_len;
 	xform_auth->auth.key.length = param->auth_key_len;
 	xform_auth->auth.key.data = param->auth_key_buf;
-- 
2.25.2


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

* [dpdk-dev] [PATCH 4/6] vhost: fix translated address not checked
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
                   ` (2 preceding siblings ...)
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 3/6] vhost/crypto: validate keys lengths Ferruh Yigit
@ 2020-05-18 13:17 ` Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 5/6] vhost: fix potential memory space leak Ferruh Yigit
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:17 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Marvin Liu, stable, Maxime Coquelin

From: Marvin Liu <yong.liu@intel.com>

Malicious guest can construct desc with invalid address and zero buffer
length. That will request vhost to check both translated address and
translated data length. This patch will add missed address check.

CVE-2020-10725
Fixes: 75ed51697820 ("vhost: add packed ring batch dequeue")
Fixes: ef861692c398 ("vhost: add packed ring batch enqueue")
Cc: stable@dpdk.org

Signed-off-by: Marvin Liu <yong.liu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 1fc30c6819..8504897e7a 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -1072,6 +1072,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev,
 						  VHOST_ACCESS_RW);
 
 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
+		if (unlikely(!desc_addrs[i]))
+			return -1;
 		if (unlikely(lens[i] != descs[avail_idx + i].len))
 			return -1;
 	}
@@ -1827,6 +1829,8 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev,
 	}
 
 	vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
+		if (unlikely(!desc_addrs[i]))
+			return -1;
 		if (unlikely((lens[i] != descs[avail_idx + i].len)))
 			return -1;
 	}
-- 
2.25.2


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

* [dpdk-dev] [PATCH 5/6] vhost: fix potential memory space leak
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
                   ` (3 preceding siblings ...)
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 4/6] vhost: fix translated address not checked Ferruh Yigit
@ 2020-05-18 13:17 ` Ferruh Yigit
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 6/6] vhost: fix potential fd leak Ferruh Yigit
  2020-05-18 14:18 ` [dpdk-dev] [PATCH 0/6] Fix vhost security issues David Marchand
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:17 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Xiaolong Ye, stable, Maxime Coquelin

From: Xiaolong Ye <xiaolong.ye@intel.com>

A malicious container which has direct access to the vhost-user socket
can keep sending VHOST_USER_GET_INFLIGHT_FD messages which may cause
leaking resources until resulting a DOS. Fix it by unmapping the
dev->inflight_info->addr before assigning new mapped addr to it.

CVE-2020-10726
Fixes: d87f1a1cb7b6 ("vhost: support inflight info sharing")
Cc: stable@dpdk.org

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index e51a8a6b77..0424e49cb8 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1433,6 +1433,11 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
 	}
 	memset(addr, 0, mmap_size);
 
+	if (dev->inflight_info->addr) {
+		munmap(dev->inflight_info->addr, dev->inflight_info->size);
+		dev->inflight_info->addr = NULL;
+	}
+
 	dev->inflight_info->addr = addr;
 	dev->inflight_info->size = msg->payload.inflight.mmap_size = mmap_size;
 	dev->inflight_info->fd = msg->fds[0] = fd;
@@ -1517,8 +1522,10 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev, VhostUserMsg *msg,
 		}
 	}
 
-	if (dev->inflight_info->addr)
+	if (dev->inflight_info->addr) {
 		munmap(dev->inflight_info->addr, dev->inflight_info->size);
+		dev->inflight_info->addr = NULL;
+	}
 
 	addr = mmap(0, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED,
 		    fd, mmap_offset);
-- 
2.25.2


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

* [dpdk-dev] [PATCH 6/6] vhost: fix potential fd leak
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
                   ` (4 preceding siblings ...)
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 5/6] vhost: fix potential memory space leak Ferruh Yigit
@ 2020-05-18 13:17 ` Ferruh Yigit
  2020-05-18 14:18 ` [dpdk-dev] [PATCH 0/6] Fix vhost security issues David Marchand
  6 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2020-05-18 13:17 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Xuan Ding, stable, Xiaolong Ye, Maxime Coquelin

From: Xuan Ding <xuan.ding@intel.com>

Vhost will create temporary file when receiving VHOST_USER_GET_INFLIGHT_FD
message. Malicious guest can send endless this message to drain out the
resource of host.

When receiving VHOST_USER_GET_INFLIGHT_FD message repeatedly, closing the
file created during the last handling of this message.

CVE-2020-10726
Fixes: d87f1a1cb7b666550 ("vhost: support inflight info sharing")
Cc: stable@dpdk.org

Signed-off-by: Xuan Ding <xuan.ding@intel.com>
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0424e49cb8..0916f5abc0 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -206,7 +206,7 @@ vhost_backend_cleanup(struct virtio_net *dev)
 			dev->inflight_info->addr = NULL;
 		}
 
-		if (dev->inflight_info->fd > 0) {
+		if (dev->inflight_info->fd >= 0) {
 			close(dev->inflight_info->fd);
 			dev->inflight_info->fd = -1;
 		}
@@ -1408,6 +1408,7 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
 				"failed to alloc dev inflight area\n");
 			return RTE_VHOST_MSG_RESULT_ERR;
 		}
+		dev->inflight_info->fd = -1;
 	}
 
 	num_queues = msg->payload.inflight.num_queues;
@@ -1438,6 +1439,11 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
 		dev->inflight_info->addr = NULL;
 	}
 
+	if (dev->inflight_info->fd >= 0) {
+		close(dev->inflight_info->fd);
+		dev->inflight_info->fd = -1;
+	}
+
 	dev->inflight_info->addr = addr;
 	dev->inflight_info->size = msg->payload.inflight.mmap_size = mmap_size;
 	dev->inflight_info->fd = msg->fds[0] = fd;
@@ -1520,6 +1526,7 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev, VhostUserMsg *msg,
 				"failed to alloc dev inflight area\n");
 			return RTE_VHOST_MSG_RESULT_ERR;
 		}
+		dev->inflight_info->fd = -1;
 	}
 
 	if (dev->inflight_info->addr) {
@@ -1534,8 +1541,10 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev, VhostUserMsg *msg,
 		return RTE_VHOST_MSG_RESULT_ERR;
 	}
 
-	if (dev->inflight_info->fd)
+	if (dev->inflight_info->fd >= 0) {
 		close(dev->inflight_info->fd);
+		dev->inflight_info->fd = -1;
+	}
 
 	dev->inflight_info->fd = fd;
 	dev->inflight_info->addr = addr;
-- 
2.25.2


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

* Re: [dpdk-dev] [PATCH 0/6] Fix vhost security issues
  2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
                   ` (5 preceding siblings ...)
  2020-05-18 13:17 ` [dpdk-dev] [PATCH 6/6] vhost: fix potential fd leak Ferruh Yigit
@ 2020-05-18 14:18 ` David Marchand
  6 siblings, 0 replies; 8+ messages in thread
From: David Marchand @ 2020-05-18 14:18 UTC (permalink / raw)
  To: Ferruh Yigit, Maxime Coquelin; +Cc: dev

On Mon, May 18, 2020 at 3:17 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Marvin Liu (1):
>   vhost: fix translated address not checked
>
> Maxime Coquelin (3):
>   vhost: check log mmap offset and size overflow
>   vhost: fix vring index check
>   vhost/crypto: validate keys lengths
>
> Xiaolong Ye (1):
>   vhost: fix potential memory space leak
>
> Xuan Ding (1):
>   vhost: fix potential fd leak
>
>  lib/librte_vhost/vhost_crypto.c | 17 +++++++++++++++++
>  lib/librte_vhost/vhost_user.c   | 30 +++++++++++++++++++++++-------
>  lib/librte_vhost/virtio_net.c   |  4 ++++
>  3 files changed, 44 insertions(+), 7 deletions(-)
>

Series applied, thanks.


-- 
David Marchand


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

end of thread, other threads:[~2020-05-18 14:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 13:16 [dpdk-dev] [PATCH 0/6] Fix vhost security issues Ferruh Yigit
2020-05-18 13:16 ` [dpdk-dev] [PATCH 1/6] vhost: check log mmap offset and size overflow Ferruh Yigit
2020-05-18 13:17 ` [dpdk-dev] [PATCH 2/6] vhost: fix vring index check Ferruh Yigit
2020-05-18 13:17 ` [dpdk-dev] [PATCH 3/6] vhost/crypto: validate keys lengths Ferruh Yigit
2020-05-18 13:17 ` [dpdk-dev] [PATCH 4/6] vhost: fix translated address not checked Ferruh Yigit
2020-05-18 13:17 ` [dpdk-dev] [PATCH 5/6] vhost: fix potential memory space leak Ferruh Yigit
2020-05-18 13:17 ` [dpdk-dev] [PATCH 6/6] vhost: fix potential fd leak Ferruh Yigit
2020-05-18 14:18 ` [dpdk-dev] [PATCH 0/6] Fix vhost security issues David Marchand

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).