[PATCH 0/2] hw/nvme: Fix CVE-2021-3929 (DMA re-entrancy exploitation)

[PATCH 0/2] hw/nvme: Fix CVE-2021-3929 (DMA re-entrancy exploitation)

[Philippe Mathieu-Daudé]

Now that the DMA API allow passing MemTxAttrs argument and
returning MemTxResult (with MEMTX_BUS_ERROR in particular),
we can restrict the NVMe controller to memories (prohibitting
accesses by the DMA engine to devices) and block yet another
DMA re-entrancy attack.

I'll will try to get a reproducer (and authorization to commit
it as qtest) from the reporter.

Based-on: <20211216123558.799425-1-philmd@redhat.com>
"hw: Have DMA API take MemTxAttrs arg & propagate MemTxResult (part 2)"
https://lore.kernel.org/qemu-devel/20211216123558.799425-1-philmd@redhat.com/

Philippe Mathieu-Daudé (2):
  hw/nvme/ctrl: Do not ignore DMA access errors
  hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)

 hw/nvme/ctrl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.33.1

[PATCH 1/2] hw/nvme/ctrl: Do not ignore DMA access errors

[Philippe Mathieu-Daudé]

dma_buf_read/dma_buf_write() return a MemTxResult type.
Do not discard it, propagate the DMA error to the caller.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvme/ctrl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fa410a179a6..604ed0aea0d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1147,15 +1147,16 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg, uint8_t *ptr, uint32_t len,
 
     if (sg->flags & NVME_SG_DMA) {
         const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+        MemTxResult res;
         uint64_t residual;
 
         if (dir == NVME_TX_DIRECTION_TO_DEVICE) {
-            dma_buf_write(ptr, len, &residual, &sg->qsg, attrs);
+            res = dma_buf_write(ptr, len, &residual, &sg->qsg, attrs);
         } else {
-            dma_buf_read(ptr, len, &residual, &sg->qsg, attrs);
+            res = dma_buf_read(ptr, len, &residual, &sg->qsg, attrs);
         }
 
-        if (unlikely(residual)) {
+        if (unlikely(residual) || res != MEMTX_OK) {
             trace_pci_nvme_err_invalid_dma();
             return NVME_INVALID_FIELD | NVME_DNR;
         }
-- 
2.33.1

Re: [PATCH 1/2] hw/nvme/ctrl: Do not ignore DMA access errors

[Keith Busch]

On Thu, Dec 16, 2021 at 06:55:09PM +0100, Philippe Mathieu-Daudé wrote:
> dma_buf_read/dma_buf_write() return a MemTxResult type.
> Do not discard it, propagate the DMA error to the caller.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

[PATCH 2/2] hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)

[Philippe Mathieu-Daudé]

Async DMA requests might access MMIO regions and re-program the
NVMe controller internal registers while DMA requests are still
scheduled or in flight. Avoid that by prohibing the controller
to access non-memories regions.

The bug has been audited looking at the following report from
Qiuhao Li:

  =================================================================
  ==793444==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000026198
  WRITE of size 2 at 0x616000026198 thread T0
      #0 0x55d64d672178 in nvme_process_sq hw/nvme/ctrl.c:5556:25
      #1 0x55d64f3b3fde in timerlist_run_timers util/qemu-timer.c:573:9
      #2 0x55d64f3b430c in qemu_clock_run_timers util/qemu-timer.c:587:12

  0x616000026198 is located 24 bytes inside of 624-byte region [0x616000026180,0x6160000263f0)
  freed by thread T0 here:
      #1 0x7f9e20a0ddac in g_free (/lib64/libglib-2.0.so.0+0x56dac)
      #2 0x55d64d661ec2 in nvme_ctrl_reset hw/nvme/ctrl.c:5578:13
      #3 0x55d64d65b5e4 in nvme_write_bar hw/nvme/ctrl.c:5824:13
      #4 0x55d64d658f70 in nvme_mmio_write hw/nvme/ctrl.c:6174:9
      #5 0x55d64e36f413 in memory_region_write_accessor softmmu/memory.c:492:5
      #6 0x55d64e36ed51 in access_with_adjusted_size softmmu/memory.c:554:18
      #7 0x55d64e36d666 in memory_region_dispatch_write softmmu/memory.c:1504:16
      #8 0x55d64e33e8ee in flatview_write_continue softmmu/physmem.c:2812:23
      #9 0x55d64e32d0eb in flatview_write softmmu/physmem.c:2854:12
      #10 0x55d64e32cba8 in address_space_write softmmu/physmem.c:2950:18
      #11 0x55d64e32d417 in address_space_rw softmmu/physmem.c:2960:16
      #12 0x55d64cd207e2 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
      #13 0x55d64cd2054a in dma_memory_rw include/sysemu/dma.h:132:12
      #14 0x55d64cd1c922 in dma_buf_rw softmmu/dma-helpers.c:312:16
      #15 0x55d64cd1c2e1 in dma_buf_read softmmu/dma-helpers.c:327:12
      #16 0x55d64d638aab in nvme_tx hw/nvme/ctrl.c:1156:19
      #17 0x55d64d6a72f4 in nvme_c2h hw/nvme/ctrl.c:1191:12
      #18 0x55d64d6b7554 in nvme_fw_log_info hw/nvme/ctrl.c:4142:12
      #19 0x55d64d6ab5e8 in nvme_get_log hw/nvme/ctrl.c:4294:16
      #20 0x55d64d6740d5 in nvme_admin_cmd hw/nvme/ctrl.c:5499:16
      #21 0x55d64d6720a3 in nvme_process_sq hw/nvme/ctrl.c:5554:13
      #22 0x55d64f3b3fde in timerlist_run_timers util/qemu-timer.c:573:9

  previously allocated by thread T0 here:
      #1 0x7f9e20a115e0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a5e0)
      #2 0x55d64d661856 in nvme_start_ctrl hw/nvme/ctrl.c:5718:5
      #3 0x55d64d65b503 in nvme_write_bar hw/nvme/ctrl.c:5815:17
      #4 0x55d64d658f70 in nvme_mmio_write hw/nvme/ctrl.c:6174:9
      #5 0x55d64e36f413 in memory_region_write_accessor softmmu/memory.c:492:5
      #6 0x55d64e36ed51 in access_with_adjusted_size softmmu/memory.c:554:18
      #7 0x55d64e36d666 in memory_region_dispatch_write softmmu/memory.c:1504:16
      #8 0x55d64e33e8ee in flatview_write_continue softmmu/physmem.c:2812:23
      #9 0x55d64e32d0eb in flatview_write softmmu/physmem.c:2854:12
      #10 0x55d64e32cba8 in address_space_write softmmu/physmem.c:2950:18

  SUMMARY: AddressSanitizer: heap-use-after-free hw/nvme/ctrl.c:5556:25 in nvme_process_sq
  Shadow bytes around the buggy address:
    0x0c2c7fffcbe0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcbf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  =>0x0c2c7fffcc30: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2c7fffcc70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
    0x0c2c7fffcc80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Heap left redzone:       fa
    Freed heap region:       fd
  ==793444==ABORTING

Fixes: CVE-2021-3929
Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 604ed0aea0d..2be2c340b34 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1146,7 +1146,7 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg, uint8_t *ptr, uint32_t len,
     assert(sg->flags & NVME_SG_ALLOC);
 
     if (sg->flags & NVME_SG_DMA) {
-        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+        const MemTxAttrs attrs = { .memory = true };
         MemTxResult res;
         uint64_t residual;
 
-- 
2.33.1

Re: [PATCH 2/2] hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)

[Keith Busch]

On Thu, Dec 16, 2021 at 06:55:10PM +0100, Philippe Mathieu-Daudé wrote:
> Async DMA requests might access MMIO regions and re-program the
> NVMe controller internal registers while DMA requests are still
> scheduled or in flight. Avoid that by prohibing the controller
> to access non-memories regions.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 2/2] hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)

[Mauro Matteo Cascella]

On Thu, Dec 16, 2021 at 6:55 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Fixes: CVE-2021-3929

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2020298

> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvme/ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 604ed0aea0d..2be2c340b34 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1146,7 +1146,7 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg, uint8_t *ptr, uint32_t len,
>      assert(sg->flags & NVME_SG_ALLOC);
>
>      if (sg->flags & NVME_SG_DMA) {
> -        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> +        const MemTxAttrs attrs = { .memory = true };
>          MemTxResult res;
>          uint64_t residual;
>
> --
> 2.33.1
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH 0/2] hw/nvme: Fix CVE-2021-3929 (DMA re-entrancy exploitation)

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

On Dec 16 18:55, Philippe Mathieu-Daudé wrote:
> Now that the DMA API allow passing MemTxAttrs argument and
> returning MemTxResult (with MEMTX_BUS_ERROR in particular),
> we can restrict the NVMe controller to memories (prohibitting
> accesses by the DMA engine to devices) and block yet another
> DMA re-entrancy attack.
> 
> I'll will try to get a reproducer (and authorization to commit
> it as qtest) from the reporter.
> 
> Based-on: <20211216123558.799425-1-philmd@redhat.com>
> "hw: Have DMA API take MemTxAttrs arg & propagate MemTxResult (part 2)"
> https://lore.kernel.org/qemu-devel/20211216123558.799425-1-philmd@redhat.com/
> 
> Philippe Mathieu-Daudé (2):
>   hw/nvme/ctrl: Do not ignore DMA access errors
>   hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)
> 
>  hw/nvme/ctrl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

LGTM.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] hw/nvme: Fix CVE-2021-3929 (DMA re-entrancy exploitation)

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Dec 16 20:13, Klaus Jensen wrote:
> On Dec 16 18:55, Philippe Mathieu-Daudé wrote:
> > Now that the DMA API allow passing MemTxAttrs argument and
> > returning MemTxResult (with MEMTX_BUS_ERROR in particular),
> > we can restrict the NVMe controller to memories (prohibitting
> > accesses by the DMA engine to devices) and block yet another
> > DMA re-entrancy attack.
> > 
> > I'll will try to get a reproducer (and authorization to commit
> > it as qtest) from the reporter.
> > 
> > Based-on: <20211216123558.799425-1-philmd@redhat.com>
> > "hw: Have DMA API take MemTxAttrs arg & propagate MemTxResult (part 2)"
> > https://lore.kernel.org/qemu-devel/20211216123558.799425-1-philmd@redhat.com/
> > 
> > Philippe Mathieu-Daudé (2):
> >   hw/nvme/ctrl: Do not ignore DMA access errors
> >   hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)
> > 
> >  hw/nvme/ctrl.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> 
> LGTM.
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Ugh. Jumped the gun here.

This all looked fine, but since this prohibits DMA to other devices it
breaks DMA'ing to a controller memory buffer on another device, which is
a used feature of some setups.

I think we need to fix this like e1000 did?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] hw/nvme: Fix CVE-2021-3929 (DMA re-entrancy exploitation)

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

On Dec 16 20:55, Klaus Jensen wrote:
> On Dec 16 20:13, Klaus Jensen wrote:
> > On Dec 16 18:55, Philippe Mathieu-Daudé wrote:
> > > Now that the DMA API allow passing MemTxAttrs argument and
> > > returning MemTxResult (with MEMTX_BUS_ERROR in particular),
> > > we can restrict the NVMe controller to memories (prohibitting
> > > accesses by the DMA engine to devices) and block yet another
> > > DMA re-entrancy attack.
> > > 
> > > I'll will try to get a reproducer (and authorization to commit
> > > it as qtest) from the reporter.
> > > 
> > > Based-on: <20211216123558.799425-1-philmd@redhat.com>
> > > "hw: Have DMA API take MemTxAttrs arg & propagate MemTxResult (part 2)"
> > > https://lore.kernel.org/qemu-devel/20211216123558.799425-1-philmd@redhat.com/
> > > 
> > > Philippe Mathieu-Daudé (2):
> > >   hw/nvme/ctrl: Do not ignore DMA access errors
> > >   hw/nvme/ctrl: Prohibit DMA accesses to devices (CVE-2021-3929)
> > > 
> > >  hw/nvme/ctrl.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > 
> > LGTM.
> > 
> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> 
> Ugh. Jumped the gun here.
> 
> This all looked fine, but since this prohibits DMA to other devices it
> breaks DMA'ing to a controller memory buffer on another device, which is
> a used feature of some setups.
> 
> I think we need to fix this like e1000 did?

Something like this maybe?

This fixes CVE-2021-3929 "locally" by denying DMA to the iomem of the
device itself. This still allows DMA to MMIO regions of other devices
(e.g. doing P2P DMA to the controller memory buffer of another NVMe
device).


diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 5f573c417b3d..9a79f6728867 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -357,6 +357,16 @@ static inline void *nvme_addr_to_pmr(NvmeCtrl *n, hwaddr addr)
     return memory_region_get_ram_ptr(&n->pmr.dev->mr) + (addr - n->pmr.cba);
 }

+static inline bool nvme_addr_is_iomem(NvmeCtrl *n, hwaddr addr)
+{
+    hwaddr hi, lo;
+
+    lo = n->bar0.addr;
+    hi = lo + int128_get64(n->bar0.size);
+
+    return addr >= lo && addr < hi;
+}
+
 static int nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
 {
     hwaddr hi = addr + size - 1;
@@ -614,6 +624,10 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, NvmeSg *sg, hwaddr addr, size_t len)

     trace_pci_nvme_map_addr(addr, len);

+    if (nvme_addr_is_iomem(n, addr)) {
+        return NVME_DATA_TRAS_ERROR;
+    }
+
     if (nvme_addr_is_cmb(n, addr)) {
         cmb = true;
     } else if (nvme_addr_is_pmr(n, addr)) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]