Skip to contentSkip to conversation

nvme: check size before memcpy

PATCH In review P J P 2 participants 2 messages Oct 22, 2018 – Oct 26, 2018

lore
Share & tools
lore

P J P

author
#0 lore raw

PATCH 2/3 nvme: check size before memcpy

From: Prasad J Pandit <[email protected]>

While in nvme_mmio_read, memcpy could read past the 'n->bar' buffer, if addr offset was pointing towards its tail end. Add check to avoid OOB access.

Reported-by Caihongzhu <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/block/nvme.c +1 -1
index fc7dacb816..87afc19b61 100644
@@ -1059,7 +1059,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
         /* should RAZ, fall through for now */
     }
 
-    if (addr < sizeof(n->bar)) {
+    if (addr + size <= sizeof(n->bar)) {
         memcpy(&val, ptr + addr, size);
     } else {
         NVME_GUEST_ERR(nvme_ub_mmiord_invalid_ofs,
Signature · 1 line
2.17.2
#1 lore raw
Quoted 18 lines · P J P

From: Prasad J Pandit <[email protected]>

While in nvme_mmio_read, memcpy could read past the 'n->bar' buffer, if addr offset was pointing towards its tail end. Add check to avoid OOB access.

Reported-by Caihongzhu <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/block/nvme.c +0 -0
index fc7dacb816..87afc19b61 100644

hw/block/nvme.c static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) · line 1065

Quoted 8 lines
hw/block/nvme.c +1 -1
@@ -1059,7 +1059,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
         /* should RAZ, fall through for now */
     }
 
-    if (addr < sizeof(n->bar)) {
+    if (addr + size <= sizeof(n->bar)) {
         memcpy(&val, ptr + addr, size);
     } else {
         NVME_GUEST_ERR(nvme_ub_mmiord_invalid_ofs,

Do you have a reproducer? In particular, I think this cannot happen because memory_region_dispatch_read will block accesses beyond 4 bytes, and earlier code in this function already check that accesses are aligned to 32 bits.

We could clarify it with

hw/block/nvme.c +1 -1
index fc7dacb816..427e69a78d 100644
@@ -1167,7 +1167,7 @@ static const MemoryRegionOps nvme_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 2,
-        .max_access_size = 8,
+        .max_access_size = 4,
     },
 };

but if my understanding is right then there is no bug.

Paolo