1. Patchew
  2. QEMU
  3. View series

[PATCH] msix: add valid.accepts methods to check address

P J P posted 1 patch 5 years, 8 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200601051454.826415-1-ppandit@redhat.com
Maintainers: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>
hw/pci/msix.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
[PATCH] msix: add valid.accepts methods to check address
Posted by P J P 5 years, 8 months ago 
From: Prasad J Pandit <pjp@fedoraproject.org>

While doing msi-x mmio operations, a guest may send an address
that leads to an OOB access issue. Add valid.accepts methods to
ensure that ensuing mmio r/w operation don't go beyond regions.

Reported-by: Ren Ding <rding@gatech.edu>
Reported-by: Hanqing Zhao <hanqing@gatech.edu>
Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/pci/msix.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 29187898f2..d90d66a3b8 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -193,6 +193,15 @@ static void msix_table_mmio_write(void *opaque, hwaddr addr,
     msix_handle_mask_update(dev, vector, was_masked);
 }
 
+static bool msix_table_accepts(void *opaque, hwaddr addr, unsigned size,
+                                    bool is_write, MemTxAttrs attrs)
+{
+    PCIDevice *dev = opaque;
+    uint16_t tbl_size = dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;
+
+    return dev->msix_table + addr + 4 <= dev->msix_table + tbl_size;
+}
+
 static const MemoryRegionOps msix_table_mmio_ops = {
     .read = msix_table_mmio_read,
     .write = msix_table_mmio_write,
@@ -200,6 +209,7 @@ static const MemoryRegionOps msix_table_mmio_ops = {
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
+        .accepts = msix_table_accepts
     },
 };
 
@@ -221,6 +231,15 @@ static void msix_pba_mmio_write(void *opaque, hwaddr addr,
 {
 }
 
+static bool msix_pba_accepts(void *opaque, hwaddr addr, unsigned size,
+                                    bool is_write, MemTxAttrs attrs)
+{
+    PCIDevice *dev = opaque;
+    uint16_t pba_size = QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;
+
+    return dev->msix_pba + addr + 4 <= dev->msix_pba + pba_size;
+}
+
 static const MemoryRegionOps msix_pba_mmio_ops = {
     .read = msix_pba_mmio_read,
     .write = msix_pba_mmio_write,
@@ -228,6 +247,7 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
+        .accepts = msix_pba_accepts
     },
 };
 
-- 
2.26.2
Re: [PATCH] msix: add valid.accepts methods to check address
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago 
On 6/1/20 7:14 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While doing msi-x mmio operations, a guest may send an address
> that leads to an OOB access issue. Add valid.accepts methods to
> ensure that ensuing mmio r/w operation don't go beyond regions.
> 

Fixes: CVE-2020-xxxxx

> Reported-by: Ren Ding <rding@gatech.edu>
> Reported-by: Hanqing Zhao <hanqing@gatech.edu>
> Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/pci/msix.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 29187898f2..d90d66a3b8 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -193,6 +193,15 @@ static void msix_table_mmio_write(void *opaque, hwaddr addr,
>      msix_handle_mask_update(dev, vector, was_masked);
>  }
>  
> +static bool msix_table_accepts(void *opaque, hwaddr addr, unsigned size,
> +                                    bool is_write, MemTxAttrs attrs)
> +{
> +    PCIDevice *dev = opaque;
> +    uint16_t tbl_size = dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;
> +
> +    return dev->msix_table + addr + 4 <= dev->msix_table + tbl_size;

Can be simplified as:

       return addr + 4 <= dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;

> +}
> +
>  static const MemoryRegionOps msix_table_mmio_ops = {
>      .read = msix_table_mmio_read,
>      .write = msix_table_mmio_write,
> @@ -200,6 +209,7 @@ static const MemoryRegionOps msix_table_mmio_ops = {
>      .valid = {
>          .min_access_size = 4,
>          .max_access_size = 4,
> +        .accepts = msix_table_accepts
>      },
>  };
>  
> @@ -221,6 +231,15 @@ static void msix_pba_mmio_write(void *opaque, hwaddr addr,
>  {
>  }
>  
> +static bool msix_pba_accepts(void *opaque, hwaddr addr, unsigned size,
> +                                    bool is_write, MemTxAttrs attrs)
> +{
> +    PCIDevice *dev = opaque;
> +    uint16_t pba_size = QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;
> +
> +    return dev->msix_pba + addr + 4 <= dev->msix_pba + pba_size;

Can be simplified as:

       return addr + 4 <= QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;

> +}
> +
>  static const MemoryRegionOps msix_pba_mmio_ops = {
>      .read = msix_pba_mmio_read,
>      .write = msix_pba_mmio_write,
> @@ -228,6 +247,7 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
>      .valid = {
>          .min_access_size = 4,
>          .max_access_size = 4,
> +        .accepts = msix_pba_accepts
>      },
>  };
>  
>
Re: [PATCH] msix: add valid.accepts methods to check address
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago 
On Mon, Jun 1, 2020 at 8:02 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 6/1/20 7:14 AM, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> >
> > While doing msi-x mmio operations, a guest may send an address
> > that leads to an OOB access issue. Add valid.accepts methods to
> > ensure that ensuing mmio r/w operation don't go beyond regions.
> >
>
> Fixes: CVE-2020-xxxxx

Oh and please also add:

Cc: qemu-stable@nongnu.org

>
> > Reported-by: Ren Ding <rding@gatech.edu>
> > Reported-by: Hanqing Zhao <hanqing@gatech.edu>
> > Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/pci/msix.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index 29187898f2..d90d66a3b8 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -193,6 +193,15 @@ static void msix_table_mmio_write(void *opaque, hwaddr addr,
> >      msix_handle_mask_update(dev, vector, was_masked);
> >  }
> >
> > +static bool msix_table_accepts(void *opaque, hwaddr addr, unsigned size,
> > +                                    bool is_write, MemTxAttrs attrs)
> > +{
> > +    PCIDevice *dev = opaque;
> > +    uint16_t tbl_size = dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;
> > +
> > +    return dev->msix_table + addr + 4 <= dev->msix_table + tbl_size;
>
> Can be simplified as:
>
>        return addr + 4 <= dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;
>
> > +}
> > +
> >  static const MemoryRegionOps msix_table_mmio_ops = {
> >      .read = msix_table_mmio_read,
> >      .write = msix_table_mmio_write,
> > @@ -200,6 +209,7 @@ static const MemoryRegionOps msix_table_mmio_ops = {
> >      .valid = {
> >          .min_access_size = 4,
> >          .max_access_size = 4,
> > +        .accepts = msix_table_accepts
> >      },
> >  };
> >
> > @@ -221,6 +231,15 @@ static void msix_pba_mmio_write(void *opaque, hwaddr addr,
> >  {
> >  }
> >
> > +static bool msix_pba_accepts(void *opaque, hwaddr addr, unsigned size,
> > +                                    bool is_write, MemTxAttrs attrs)
> > +{
> > +    PCIDevice *dev = opaque;
> > +    uint16_t pba_size = QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;
> > +
> > +    return dev->msix_pba + addr + 4 <= dev->msix_pba + pba_size;
>
> Can be simplified as:
>
>        return addr + 4 <= QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;
>
> > +}
> > +
> >  static const MemoryRegionOps msix_pba_mmio_ops = {
> >      .read = msix_pba_mmio_read,
> >      .write = msix_pba_mmio_write,
> > @@ -228,6 +247,7 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
> >      .valid = {
> >          .min_access_size = 4,
> >          .max_access_size = 4,
> > +        .accepts = msix_pba_accepts
> >      },
> >  };
> >
> >
>
Re: [PATCH] msix: add valid.accepts methods to check address
Posted by P J P 5 years, 8 months ago 
+-- On Mon, 1 Jun 2020, Philippe Mathieu-Daudé wrote --+
| Fixes: CVE-2020-xxxxx

'CVE-2020-13754' assigned to this issue by Mitre.
  -> https://bugzilla.redhat.com/show_bug.cgi?id=1842363

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] msix: add valid.accepts methods to check address
Posted by Michael S. Tsirkin 5 years, 8 months ago 
On Mon, Jun 01, 2020 at 10:44:54AM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While doing msi-x mmio operations, a guest may send an address
> that leads to an OOB access issue. Add valid.accepts methods to
> ensure that ensuing mmio r/w operation don't go beyond regions.
> 
> Reported-by: Ren Ding <rding@gatech.edu>
> Reported-by: Hanqing Zhao <hanqing@gatech.edu>
> Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

IMHO this is just messed up, memory core needs to guarantee this.
I'm working on a patch to do that.


> ---
>  hw/pci/msix.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 29187898f2..d90d66a3b8 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -193,6 +193,15 @@ static void msix_table_mmio_write(void *opaque, hwaddr addr,
>      msix_handle_mask_update(dev, vector, was_masked);
>  }
>  
> +static bool msix_table_accepts(void *opaque, hwaddr addr, unsigned size,
> +                                    bool is_write, MemTxAttrs attrs)
> +{
> +    PCIDevice *dev = opaque;
> +    uint16_t tbl_size = dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE;
> +
> +    return dev->msix_table + addr + 4 <= dev->msix_table + tbl_size;
> +}
> +
>  static const MemoryRegionOps msix_table_mmio_ops = {
>      .read = msix_table_mmio_read,
>      .write = msix_table_mmio_write,
> @@ -200,6 +209,7 @@ static const MemoryRegionOps msix_table_mmio_ops = {
>      .valid = {
>          .min_access_size = 4,
>          .max_access_size = 4,
> +        .accepts = msix_table_accepts
>      },
>  };
>  
> @@ -221,6 +231,15 @@ static void msix_pba_mmio_write(void *opaque, hwaddr addr,
>  {
>  }
>  
> +static bool msix_pba_accepts(void *opaque, hwaddr addr, unsigned size,
> +                                    bool is_write, MemTxAttrs attrs)
> +{
> +    PCIDevice *dev = opaque;
> +    uint16_t pba_size = QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8;
> +
> +    return dev->msix_pba + addr + 4 <= dev->msix_pba + pba_size;
> +}
> +
>  static const MemoryRegionOps msix_pba_mmio_ops = {
>      .read = msix_pba_mmio_read,
>      .write = msix_pba_mmio_write,
> @@ -228,6 +247,7 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
>      .valid = {
>          .min_access_size = 4,
>          .max_access_size = 4,
> +        .accepts = msix_pba_accepts
>      },
>  };
>  
> -- 
> 2.26.2
Re: [PATCH] msix: add valid.accepts methods to check address
Posted by P J P 5 years, 8 months ago 
+-- On Mon, 1 Jun 2020, Michael S. Tsirkin wrote --+
| IMHO this is just messed up, memory core needs to guarantee this.
| I'm working on a patch to do that.

Okay. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.