Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len

[Martin Radev <martin.b.radev@gmail.com>]

Thanks Alex.

I needed to make a small update to the patch as you recommented in one
of the other emails. I still kept the reviewed by line with your name.

From 090f373c0bc868cc4551620568d47b21b6ac044a Mon Sep 17 00:00:00 2001
From: Martin Radev <martin.b.radev@gmail.com>
Date: Mon, 17 Jan 2022 23:17:25 +0200
Subject: [PATCH kvmtool 2/6] mmio: Sanitize addr and len

This patch verifies that adding the addr and length arguments
from an MMIO op do not overflow. This is necessary because the
arguments are controlled by the VM. The length may be set to
an arbitrary value by using the rep prefix.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 mmio.c        | 4 ++++
 virtio/mmio.c | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/mmio.c b/mmio.c
index a6dd3aa..5a114e9 100644
--- a/mmio.c
+++ b/mmio.c
@@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
 {
 	struct rb_int_node *node;
 
+	/* If len is zero or if there's an overflow, the MMIO op is invalid. */
+	if (addr + len <= addr)
+		return NULL;
+
 	node = rb_int_search_range(root, addr, addr + len);
 	if (node == NULL)
 		return NULL;
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 875a288..979fa8c 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -105,6 +105,12 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
 	struct virtio_mmio *vmmio = vdev->virtio;
 	u32 i;
 
+	/* Check for wrap-around and zero length. */
+	if (addr + len <= addr) {
+		WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
+		return;
+	}
+
 	for (i = 0; i < len; i++) {
 		if (is_write)
 			vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
-- 
2.25.1

On Wed, Mar 16, 2022 at 03:39:55PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Mar 04, 2022 at 01:10:50AM +0200, Martin Radev wrote:
> > This patch verifies that adding the addr and length arguments
> > from an MMIO op do not overflow. This is necessary because the
> > arguments are controlled by the VM. The length may be set to
> > an arbitrary value by using the rep prefix.
> > 
> > Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> 
> The patch looks correct to me:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> Thanks,
> Alex
> 
> > ---
> >  mmio.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/mmio.c b/mmio.c
> > index a6dd3aa..5a114e9 100644
> > --- a/mmio.c
> > +++ b/mmio.c
> > @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
> >  {
> >  	struct rb_int_node *node;
> >  
> > +	/* If len is zero or if there's an overflow, the MMIO op is invalid. */
> > +	if (addr + len <= addr)
> > +		return NULL;
> > +
> >  	node = rb_int_search_range(root, addr, addr + len);
> >  	if (node == NULL)
> >  		return NULL;
> > -- 
> > 2.25.1
> >