Hi,
On Mon, Mar 28, 2022 at 12:00:30AM +0300, Martin Radev wrote:
> 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.
That's quite fine, please send it in a new iteration of the series.
Thanks,
Alex
>
> From 090f373c0bc868cc4551620568d47b21b6ac044a Mon Sep 17 00:00:00 2001
> From: Martin Radev <martin.b.radev@xxxxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Martin Radev <martin.b.radev@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> >
> > The patch looks correct to me:
> >
> > Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> >
> > 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
> > >
