diff mbox series

hw/nvme: fix CVE-2021-3929

Message ID 20220120080155.227246-1-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series hw/nvme: fix CVE-2021-3929 | expand

Commit Message

Klaus Jensen Jan. 20, 2022, 8:01 a.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

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

Fixes: CVE-2021-3929
Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Keith Busch Jan. 20, 2022, 3:10 p.m. UTC | #1 
On Thu, Jan 20, 2022 at 09:01:55AM +0100, Klaus Jensen wrote:
> +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;

Looks fine considering this implementation always puts CMB in an
exclusive BAR. From a spec consideration though, you can put a CMB at a
BAR0 offset. I don't think that's going to happen anytime soon here, but
may be worth a comment to notify this function needs to be updated if
that assumption ever changes.

Reviewed-by: Keith Busch <kbusch@kernel.org>
Klaus Jensen Jan. 20, 2022, 8:13 p.m. UTC | #2 
On Jan 20 07:10, Keith Busch wrote:
> On Thu, Jan 20, 2022 at 09:01:55AM +0100, Klaus Jensen wrote:
> > +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;
> 
> Looks fine considering this implementation always puts CMB in an
> exclusive BAR. From a spec consideration though, you can put a CMB at a
> BAR0 offset. I don't think that's going to happen anytime soon here, but
> may be worth a comment to notify this function needs to be updated if
> that assumption ever changes.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>

Nice catch Keith. A comment would be wise!
diff mbox series

Patch

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