nvme: fix oob access issue(CVE-2018-16847)

PATCH In review Li Qiang 5 participants 18 messages Nov 2, 2018 – Nov 15, 2018

Share & tools

Li Qiang

author

2018-11-02 01:27 UTC #0 lore raw

PATCH 1/1 nvme: fix oob access issue(CVE-2018-16847)

Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.

Fixes CVE-2018-16847.

Reported-by Li Qiang <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Li Qiang <[email protected]>

---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/block/nvme.c +7 -0

index fc7dacb..d097add 100644
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data,
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+        return;
+    }
     memcpy(&n->cmbuf[addr], &data, size);
 }

@@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr addr, unsigned size)
     uint64_t val;
     NvmeCtrl *n = (NvmeCtrl *)opaque;
 
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+        return 0;
+    }
     memcpy(&val, &n->cmbuf[addr], size);
     return val;
 }

Signature · 1 line

1.8.3.1

Philippe Mathieu-Daudé

2018-11-02 07:52 UTC #1 lore raw

Quoted 18 lines · Li Qiang

Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
  hw/block/nvme.c | 7 +++++++
  1 file changed, 7 insertions(+)
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644

hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data, · line 1179

Quoted 5 lines

hw/block/nvme.c +2 -0

@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data,
      unsigned size)
  {
      NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {

Should this be reported via qemu_log_mask(LOG_GUEST_ERROR, ...)?

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

Quoted 10 lines

hw/block/nvme.c +3 -0

+        return;
+    }
      memcpy(&n->cmbuf[addr], &data, size);
  }

@@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr addr, unsigned size)
      uint64_t val;
      NvmeCtrl *n = (NvmeCtrl *)opaque;
 
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {

Ditto.

Quoted 6 lines

hw/block/nvme.c +2 -0

+        return 0;
+    }
      memcpy(&val, &n->cmbuf[addr], size);
      return val;
  }

Kevin Wolf

2018-11-02 10:54 UTC #2 lore raw

In reply to Li Qiang

Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:

Quoted 18 lines

Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644

hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data, · line 1179

Quoted 5 lines

hw/block/nvme.c +2 -0

@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data,
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {

What prevents a guest from moving the device to the end of the address space and causing an integer overflow in addr + size?

If this happens, we still have .max_access_size = 8. The next question is then, is NVME_CMBSZ_GETSIZE guaranteed to be at least 8? I suppose yes, but do we want to rely on this for security?

Kevin

Keith Busch

2018-11-02 15:02 UTC #3 lore raw

Quoted 28 lines · Kevin Wolf

Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
Quoted 18 lines
Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644
hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data, · line 1179
Quoted 5 lines
hw/block/nvme.c +2 -0
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data,
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
What prevents a guest from moving the device to the end of the address space and causing an integer overflow in addr + size?

Quoted 5 lines

If this happens, we still have .max_access_size = 8. The next question is then, is NVME_CMBSZ_GETSIZE guaranteed to be at least 8? I suppose yes, but do we want to rely on this for security?
Kevin

The nvme spec at least doesn't allow a way to express a CMB that isn't at a minimum 4k aligned and sized, so 8 byte access should always be within the boundary.

Li Qiang

author

2018-11-02 15:23 UTC #4 lore raw

In reply to Kevin Wolf

Hello Kevin,

Quoted 20 lines · Kevin Wolf <[email protected]> 于2018年11月2日周五下午6:54写道：

Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr
Quoted 18 lines
Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr
addr, uint64_t data,

Quoted 10 lines

hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr addr, uint64_t data, · line 1179
Quoted 5 lines
hw/block/nvme.c +2 -0
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
What prevents a guest from moving the device to the end of the address space and causing an integer overflow in addr + size?

This can't happen as the addr can't be any value, it just can be in the Memory Region n->ctrl_mem defines.

Thanks,
Li Qiang

Quoted 3 lines

If this happens, we still have .max_access_size = 8. The next question is then, is NVME_CMBSZ_GETSIZE guaranteed to be at least 8? I suppose yes, but do we want to rely on this for security?

Kevin

Quoted 1 line

Kevin Wolf

2018-11-02 15:42 UTC #5 lore raw

Am 02.11.2018 um 16:22 hat Li Qiang geschrieben:

Quoted 30 lines

Hello Kevin,
Quoted 26 lines · Kevin Wolf <[email protected]> 于2018年11月2日周五下午6:54写道：
Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
hw/block/nvme.c static void nvme_cmb_write(void *opaque, hwaddr
Quoted 18 lines
Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr
addr, uint64_t data,
Quoted 5 lines
hw/block/nvme.c +2 -0
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {

Quoted 6 lines

Quoted 4 lines
What prevents a guest from moving the device to the end of the address space and causing an integer overflow in addr + size?
This can't happen as the addr can't be any value, it just can be in the Memory Region n->ctrl_mem defines.

Yes, but can't the guest map that memory region whereever it wants?

(As Keith confirmed, the integer overflow doesn't seem to have any bad consequences here, but anyway.)

Kevin

Li Qiang

author

2018-11-05 01:49 UTC #6 lore raw

Quoted 32 lines · Kevin Wolf <[email protected]> 于2018年11月2日周五下午11:42写道：

Am 02.11.2018 um 16:22 hat Li Qiang geschrieben:
Quoted 18 lines
Hello Kevin,
Quoted 14 lines · Kevin Wolf <[email protected]> 于2018年11月2日周五下午6:54写道：
Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
Quoted 13 lines
Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>
---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)
Quoted 5 lines
Quoted 5 lines
hw/block/nvme.c static void nvme_cmb_write(void *opaque,
Quoted 5 lines
hw/block/nvme.c +0 -0
index fc7dacb..d097add 100644
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque,
hwaddr
Quoted 7 lines
Quoted 7 lines
addr, uint64_t data,
Quoted 5 lines
hw/block/nvme.c +2 -0
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
What prevents a guest from moving the device to the end of the address space and causing an integer overflow in addr + size?
This can't happen as the addr can't be any value, it just can be in the Memory Region n->ctrl_mem defines.
Yes, but can't the guest map that memory region whereever it wants?

I think it can't happen, as the 'addr' here is the relative offset in the MR.
As we don't (can't) register such a MMIO region(the end of this region is very big to
make addr+size overflow, size here can only be 1,2, 4, 8,). So the 'addr' here can't be that large.

Thanks,
Li Qiang

Quoted 5 lines

(As Keith confirmed, the integer overflow doesn't seem to have any bad consequences here, but anyway.)
Kevin

Keith Busch

2018-11-02 15:42 UTC #7 lore raw

In reply to Li Qiang

Quoted 9 lines · Li Qiang

Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>

Hey, so why is this memory region access even considered valid if the request is out of range from what NVMe had registered for its MemoryRegion? Wouldn't it be better to not call the mr->ops->read/write if it's out of bounds? Otherwise every MemoryRegion needs to duplicate the same check, right?

Would something like the following work (minimally tested)?

---

memory.c +3 -0

index 9b73892768..883fd818e6 100644
@@ -1369,6 +1369,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
         access_size_max = 4;
     }
 
+    if (addr + size > mr->size)
+        return false;
+
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     for (i = 0; i < size; i += access_size) {
         if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,

Li Qiang

author

2018-11-05 01:56 UTC #8 lore raw

Quoted 11 lines · Keith Busch <[email protected]> 于2018年11月2日周五下午11:42写道：

Quoted 9 lines · Li Qiang
Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by Li Qiang <[email protected]>
Reviewed-by Paolo Bonzini <[email protected]>
Signed-off-by Li Qiang <[email protected]>

Quoted 7 lines

Hey, so why is this memory region access even considered valid if the request is out of range from what NVMe had registered for its MemoryRegion? Wouldn't it be better to not call the mr->ops->read/write if it's out of bounds? Otherwise every MemoryRegion needs to duplicate the same check, right?

Yes, This seems a good idea. Once I also encounter one issue like this. The read callback in MR will be called even it is NULL so cause a NULL-deref.
discussed here:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01391.html

This touchs the core design of memory subsystem I think, May Paolo or Peter can answer this.

Quoted 19 lines

Would something like the following work (minimally tested)?

---

memory.c +3 -1

index 9b73892768..883fd818e6 100644
@@ -1369,6 +1369,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
         access_size_max = 4;
     }
 
+    if (addr + size > mr->size)
+        return false;
+
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     for (i = 0; i < size; i += access_size) {
         if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
--

Paolo Bonzini

2018-11-13 18:42 UTC #9 lore raw

In reply to Keith Busch

Quoted 5 lines · Keith Busch

Hey, so why is this memory region access even considered valid if the request is out of range from what NVMe had registered for its MemoryRegion? Wouldn't it be better to not call the mr->ops->read/write if it's out of bounds? Otherwise every MemoryRegion needs to duplicate the same check, right?

Because some crazy devices have misaligned registers.

But actually this is not a problem because NVMe doesn't set ops->impl.unaligned to true, so indeed no change is needed.

Paolo

Quoted 1 line

Would something like the following work (minimally tested)?

Li Qiang

author

2018-11-13 01:46 UTC #10 lore raw

In reply to Li Qiang

Ping.... what't the status of this patch.

I see Kevin's new pr doesn't contain this patch.

Thanks,
Li Qiang

Quoted 44 lines · Li Qiang <[email protected]> 于2018年11月2日周五上午9:22写道：

Currently, the nvme_cmb_ops mr doesn't check the addr and size. This can lead an oob access issue. This is triggerable in the guest. Add check to avoid this issue.

Fixes CVE-2018-16847.

Reported-by Li Qiang <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Li Qiang <[email protected]>

---
 hw/block/nvme.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/block/nvme.c +7 -0

index fc7dacb..d097add 100644
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr
 addr, uint64_t data,
     unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+        return;
+    }
     memcpy(&n->cmbuf[addr], &data, size);
 }

@@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr
 addr, unsigned size)
     uint64_t val;
     NvmeCtrl *n = (NvmeCtrl *)opaque;
 
+    if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+        return 0;
+    }
     memcpy(&val, &n->cmbuf[addr], size);
     return val;

}
--
1.8.3.1

Kevin Wolf

2018-11-13 10:17 UTC #11 lore raw

Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:

Quoted 3 lines

Ping.... what't the status of this patch.
I see Kevin's new pr doesn't contain this patch.

Oh, I thought you said that you wanted to fix this at a higher level so that the problem is caught before even getting into nvme code? If you don't, I can apply the patch for my next pull request.

Kevin

Li Qiang

author

2018-11-13 10:25 UTC #12 lore raw

Quoted 7 lines · Kevin Wolf <[email protected]> 于2018年11月13日周二下午6:17写道：

Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
Quoted 3 lines
Ping.... what't the status of this patch.
I see Kevin's new pr doesn't contain this patch.
Oh, I thought you said that you wanted to fix this at a higher level so that the problem is caught before even getting into nvme code?

The higher level fix may need a lot of another discussion, but this fix is just as the other things
once we did.

Quoted 3 lines

If you
don't, I can apply the patch for my next pull request.

I think you can. Also, could you look at another patchset about nvme to fix some small issues.
Seems they hangs a long time.

Thanks,
Li Qiang

Quoted 3 lines

Kevin

Paolo Bonzini

2018-11-13 18:42 UTC #13 lore raw

In reply to Kevin Wolf

Quoted 8 lines · Kevin Wolf

Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
Quoted 3 lines
Ping.... what't the status of this patch.
I see Kevin's new pr doesn't contain this patch.
Oh, I thought you said that you wanted to fix this at a higher level so that the problem is caught before even getting into nvme code? If you don't, I can apply the patch for my next pull request.

As far as I know the bug doesn't exist. Li Qiang, if you have a reproducer please send it.

Prasad, please revoke the CVE.

Paolo

Li Qiang

author

2018-11-14 01:39 UTC #14 lore raw

Quoted 4 lines · Paolo Bonzini <[email protected]> 于2018年11月14日周三上午2:27写道：

Quoted 3 lines · Kevin Wolf
Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
Quoted 2 lines
Ping.... what't the status of this patch.

Quoted 10 lines

Quoted 5 lines
Quoted 1 line
I see Kevin's new pr doesn't contain this patch.
Oh, I thought you said that you wanted to fix this at a higher level so that the problem is caught before even getting into nvme code? If you don't, I can apply the patch for my next pull request.
As far as I know the bug doesn't exist. Li Qiang, if you have a reproducer please send it.

Hello Paolo,
Though I've send the debug information and ASAN output in the mail to [email protected], I'm glad provide here.
This is for read, I think the write the same but as the PoC is in userspace, the mmap can only map the exact size of the MMIO,
So we can only write within the area. But if we using a module we can write the out of MMIO I think
The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI address may differ in your system.

Thanks,
Li Qiang

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <fcntl.h>
#include <sys/mman.h>

int main(int argc, char **argv)

{
    char *filename = "/sys/bus/pci/devices/0000:00:04.0/resource2";
    uint32_t size = 2*1024*1024;
    char *mmio = NULL;
    int fd = open(filename, O_RDWR);
    if (fd < 0) {
        printf("open file error\n");
    exit(1);
    }
    mmio = mmap(NULL, size, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
    if (mmio == MAP_FAILED) {
        printf("mmap error\n");
    exit(1);
    }
    int x = *(uint64_t*)(mmio+size-1);
}

read:

[Switching to Thread 0x7fffc7326700 (LWP 52799)]

Thread 4 "qemu-system-x86" hit Breakpoint 1, nvme_cmb_read (opaque=0x6240000b8100, addr=2097151, size=2) at hw/block/nvme.c:1182 1182 {
(gdb) p /x addr
$1 = 0x1fffff
(gdb) p /x addr+size
$2 = 0x200001
(gdb) c
Continuing.
=================================================================
[Thread 0x7fff77efd700 (LWP 54057) exited]
==52793==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fff817ff800 at pc 0x7ffff6e9af7f bp 0x7fffc7322fc0 sp 0x7fffc7322770 READ of size 2 at 0x7fff817ff800 thread T3
[Thread 0x7fff7a183700 (LWP 53957) exited]
[Thread 0x7fff786fe700 (LWP 53953) exited]
[Thread 0x7fff70b21700 (LWP 53952) exited]

    #0 0x7ffff6e9af7e  (/usr/lib/x86_64-linux-gnu/libasan.so.3+0x5cf7e)
    #1 0x5555562193b3 in nvme_cmb_read hw/block/nvme.c:1186
    #2 0x555555d630d0 in memory_region_read_accessor
/home/liqiang02/qemu-upstream/qemu/memory.c:440
    #3 0x555555d638da in access_with_adjusted_size
/home/liqiang02/qemu-upstream/qemu/memory.c:570
    #4 0x555555d690fd in memory_region_dispatch_read1
/home/liqiang02/qemu-upstream/qemu/memory.c:1375
    #5 0x555555d692b5 in memory_region_dispatch_read
/home/liqiang02/qemu-upstream/qemu/memory.c:1404
    #6 0x555555ca765b in flatview_read_continue
/home/liqiang02/qemu-upstream/qemu/exec.c:3294
    #7 0x555555ca790d in flatview_read
/home/liqiang02/qemu-upstream/qemu/exec.c:3332
    #8 0x555555ca79d3 in address_space_read_full
/home/liqiang02/qemu-upstream/qemu/exec.c:3345
    #9 0x555555ca7aaa in address_space_rw
/home/liqiang02/qemu-upstream/qemu/exec.c:3375
    #10 0x555555daadd9 in kvm_cpu_exec
/home/liqiang02/qemu-upstream/qemu/accel/kvm/kvm-all.c:2031
    #11 0x555555d2b2e5 in qemu_kvm_cpu_thread_fn
/home/liqiang02/qemu-upstream/qemu/cpus.c:1277
    #12 0x555556a037a0 in qemu_thread_start util/qemu-thread-posix.c:498
    #13 0x7fffdadbd493 in start_thread
(/lib/x86_64-linux-gnu/libpthread.so.0+0x7493)
    #14 0x7fffdaafface in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xe8ace)

Quoted 5 lines

Prasad, please revoke the CVE.
Paolo

Paolo Bonzini

2018-11-14 15:44 UTC #15 lore raw

Quoted 20 lines · Li Qiang

Paolo Bonzini <[email protected] <mailto:[email protected]>> 于2018 年11月14日周三上午2:27写道：
On 13/11/2018 11:17, Kevin Wolf wrote:
> Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
>> Ping.... what't the status of this patch.
>>
>> I see Kevin's new pr doesn't contain this patch.
>
> Oh, I thought you said that you wanted to fix this at a higher level so
> that the problem is caught before even getting into nvme code? If you > don't, I can apply the patch for my next pull request.
As far as I know the bug doesn't exist. Li Qiang, if you have a reproducer please send it.

Quoted 9 lines

Hello Paolo,
Though I've send the debug information and ASAN output in the mail to [email protected] <mailto:[email protected]>, I'm glad provide here. This is for read, I think the write the same but as the PoC is in userspace, the mmap can only map the exact size of the MMIO, So we can only write within the area. But if we using a module we can write the out of MMIO I think
The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI address may differ in your system.

Ok, thanks. I've created a reproducer using qtest (though I have to run now and cannot post it properly).

The patch for the fix is simply:

hw/block/nvme.c +1 -1

index fc7dacb816..6385033af3 100644
@@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
     .write = nvme_cmb_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 2,
+        .min_access_size = 1,
         .max_access_size = 8,
     },
 };

The memory subsystem _is_ recognizing the out-of-bounds 32-bit access, but because min_access_size=2 it sends down a write at offset 2097151 and size 2.

Paolo

Li Qiang

author

2018-11-15 03:15 UTC #16 lore raw

Quoted 32 lines · Paolo Bonzini <[email protected]> 于2018年11月14日周三下午11:44写道：

Quoted 9 lines · Li Qiang
Paolo Bonzini <[email protected] <mailto:[email protected]>> 于2018 年11月14日周三上午2:27写道：
On 13/11/2018 11:17, Kevin Wolf wrote:
> Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
>> Ping.... what't the status of this patch. >>
Quoted 5 lines
>> I see Kevin's new pr doesn't contain this patch.
>
> Oh, I thought you said that you wanted to fix this at a higher level so
> that the problem is caught before even getting into nvme code? If
you
Quoted 6 lines
Quoted 1 line
don't, I can apply the patch for my next pull request.
As far as I know the bug doesn't exist. Li Qiang, if you have a reproducer please send it.
Quoted 9 lines
Hello Paolo,
Though I've send the debug information and ASAN output in the mail to [email protected] <mailto:[email protected]>, I'm glad provide here. This is for read, I think the write the same but as the PoC is in userspace, the mmap can only map the exact size of the MMIO, So we can only write within the area. But if we using a module we can write the out of MMIO I think
The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI address may differ in your system.

Quoted 6 lines

Ok, thanks. I've created a reproducer using qtest (though I have to run now and cannot post it properly).
The patch for the fix is simply:

So do you send this or me?

Quoted 16 lines

hw/block/nvme.c +1 -1

index fc7dacb816..6385033af3 100644
@@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
     .write = nvme_cmb_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 2,
+        .min_access_size = 1,
         .max_access_size = 8,
     },
 };

Quoted 2 lines

The memory subsystem _is_ recognizing the out-of-bounds 32-bit access,

Thanks, this strengthen my understanding of memory subsystem.

Thanks,
Li Qiang

Quoted 6 lines

but because min_access_size=2 it sends down a write at offset 2097151 and size 2.
Paolo

Paolo Bonzini

2018-11-15 18:10 UTC #17 lore raw

Quoted 40 lines · Li Qiang

Paolo Bonzini <[email protected] <mailto:[email protected]>> 于2018 年11月14日周三下午11:44写道：
On 14/11/2018 02:38, Li Qiang wrote:
>
>
> Paolo Bonzini <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> 于2018 > 年11月14日周三上午2:27写道：
>
> On 13/11/2018 11:17, Kevin Wolf wrote:
> > Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
> >> Ping.... what't the status of this patch.
> >>
> >> I see Kevin's new pr doesn't contain this patch.
> >
> > Oh, I thought you said that you wanted to fix this at a higher > level so
> > that the problem is caught before even getting into nvme code? If you
> > don't, I can apply the patch for my next pull request. >
> As far as I know the bug doesn't exist. Li Qiang, if you have a > reproducer please send it.
>
>
> Hello Paolo,
> Though I've send the debug information and ASAN output in the mail to > [email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>, I'm glad provide here.
> This is for read, I think the write the same but as the PoC is in > userspace, the mmap can only map the exact size of the MMIO, > So we can only write within the area. But if we using a module we can > write the out of MMIO I think
> The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI > address may differ in your system.

Quoted 7 lines

Ok, thanks. I've created a reproducer using qtest (though I have to run now and cannot post it properly).
The patch for the fix is simply:
So do you send this or me?

Me, together with the test.

Paolo