[Qemu-devel] [PATCH 2/3] nvme: check size before memcpy

[Qemu-devel] [PATCH 2/3] nvme: check size before memcpy

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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 <caihongzhu@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..87afc19b61 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -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,
-- 
2.17.2

Re: [Qemu-devel] [PATCH 2/3] nvme: check size before memcpy

[Paolo Bonzini]

On 22/10/2018 14:14, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> 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 <caihongzhu@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..87afc19b61 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..427e69a78d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -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