[Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Salva Peiró]

The memory.c file directly calls the function pointers provided in
the MemoryRegionOps to handle read and write operations for memory regions.
The function pointers are called without checking if the function
pointers are initialised, therefore, leading to QEMU SIGSEGV when the
operations are not initialised.

The patch adds explicit checks to function pointers before issuing the calls.

Signed-off-by: Salva Peiró <speirofr@gmail.com>
---
 memory.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/memory.c b/memory.c
index 5e5f325..d588c5f 100644
--- a/memory.c
+++ b/memory.c
@@ -380,6 +380,9 @@ static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->old_mmio.read) {
+        return MEMTX_ERROR;
+    }
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -396,6 +399,9 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->read) {
+        return MEMTX_ERROR;
+    }
     tmp = mr->ops->read(mr->opaque, addr, size);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -413,6 +419,9 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
     uint64_t tmp = 0;
     MemTxResult r;
 
+    if (!mr->ops->read_with_attrs) {
+        return MEMTX_ERROR;
+    }
     r = mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -429,6 +438,9 @@ static MemTxResult memory_region_oldmmio_write_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->old_mmio.write) {
+        return MEMTX_ERROR;
+    }
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
     mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
@@ -447,6 +459,9 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
 
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
+    if (!mr->ops->write) {
+        return MEMTX_ERROR;
+    }
     mr->ops->write(mr->opaque, addr, tmp, size);
     return MEMTX_OK;
 }
@@ -463,6 +478,9 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
 
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
+    if (!mr->ops->write_with_attrs) {
+        return MEMTX_ERROR;
+    }
     return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
 }
 
@@ -1187,6 +1205,8 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size)
 {
     memory_region_init(mr, owner, name, size);
+    assert(ops != NULL);
+
     mr->ops = ops;
     mr->opaque = opaque;
     mr->terminates = true;
-- 
2.1.4

[Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Salva Peiró]

The file memory.c directly calls the function pointers provided in
the MemoryRegionOps to handle read and write operations for memory regions.
The function pointers are called without checking if the function
pointers are initialised, therefore, causing QEMU to SIGSEGV when
accessing a memory address for which the operation is not defined (and not initialised)

The patch adds explicit checks to function pointers before issuing the calls.

Signed-off-by: Salva Peiró <speirofr@gmail.com>
---
 memory.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/memory.c b/memory.c
index 5e5f325..d588c5f 100644
--- a/memory.c
+++ b/memory.c
@@ -380,6 +380,9 @@ static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->old_mmio.read) {
+        return MEMTX_ERROR;
+    }
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -396,6 +399,9 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->read) {
+        return MEMTX_ERROR;
+    }
     tmp = mr->ops->read(mr->opaque, addr, size);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -413,6 +419,9 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
     uint64_t tmp = 0;
     MemTxResult r;
 
+    if (!mr->ops->read_with_attrs) {
+        return MEMTX_ERROR;
+    }
     r = mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
     trace_memory_region_ops_read(mr, addr, tmp, size);
     *value |= (tmp & mask) << shift;
@@ -429,6 +438,9 @@ static MemTxResult memory_region_oldmmio_write_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->old_mmio.write) {
+        return MEMTX_ERROR;
+    }
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
     mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
@@ -447,6 +459,9 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
 
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
+    if (!mr->ops->write) {
+        return MEMTX_ERROR;
+    }
     mr->ops->write(mr->opaque, addr, tmp, size);
     return MEMTX_OK;
 }
@@ -463,6 +478,9 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
 
     tmp = (*value >> shift) & mask;
     trace_memory_region_ops_write(mr, addr, tmp, size);
+    if (!mr->ops->write_with_attrs) {
+        return MEMTX_ERROR;
+    }
     return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
 }
 
@@ -1187,6 +1205,8 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size)
 {
     memory_region_init(mr, owner, name, size);
+    assert(ops != NULL);
+
     mr->ops = ops;
     mr->opaque = opaque;
     mr->terminates = true;
-- 
2.1.4

Re: [Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Paolo Bonzini]

On 03/09/2015 19:37, Salva Peiró wrote:
> The file memory.c directly calls the function pointers provided in
> the MemoryRegionOps to handle read and write operations for memory regions.
> The function pointers are called without checking if the function
> pointers are initialised, therefore, causing QEMU to SIGSEGV when
> accessing a memory address for which the operation is not defined (and not initialised)
> 
> The patch adds explicit checks to function pointers before issuing the calls.

What device are you encountering this for?  Perhaps this should be done
in memory_region_init_io instead, so that it is detected early.

Paolo

Re: [Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Salva Peiró]

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

On 9/7/15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 03/09/2015 19:37, Salva Peiró wrote:
>> The file memory.c directly calls the function pointers provided in
>> the MemoryRegionOps to handle read and write operations for memory
>> regions.
>> The function pointers are called without checking if the function
>> pointers are initialised, therefore, causing QEMU to SIGSEGV when
>> accessing a memory address for which the operation is not defined (and not
>> initialised)
>>
>> The patch adds explicit checks to function pointers before issuing the
>> calls.
>
> What device are you encountering this for?  Perhaps this should be done
> in memory_region_init_io instead, so that it is detected early.
>
> Paolo
>

Rigth, I should have started by providing the scenario where the fault occurs.
The problem occurs performing a writeb to the BAR0 of device 1033:194.
That is PCI_DEVICE_ID_NEC_UPD720200  0x0194 at hw/usb/hcd-xhci.c

I've attached tests/nec-usb-xhci-test.c that reproduces the scenario.

Best
--
salva

[-- Attachment #2: 0001-tests-nec-usb-xhci-test.c.patch --]
[-- Type: text/x-patch, Size: 4076 bytes --]

From e66ab9c9b9836f37866605acd2e1efda422b0e31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Salva=20Peir=C3=B3?= <speirofr@gmail.com>
Date: Tue, 8 Sep 2015 08:40:20 +0200
Subject: [PATCH] tests/nec-usb-xhci-test.c

---
 tests/Makefile            |   2 +
 tests/nec-usb-xhci-test.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 tests/nec-usb-xhci-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 34c6136..08ae505 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -416,6 +416,8 @@ tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(block-obj-y) libqemuutil.a libqemustub.a
+tests/nec-usb-xhci-test$(EXESUF): tests/nec-usb-xhci-test.o $(libqos-pc-obj-y) $(qtest-obj-y) tests/libqtest.o libqemuutil.a libqemustub.a
+
 
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil
diff --git a/tests/nec-usb-xhci-test.c b/tests/nec-usb-xhci-test.c
new file mode 100644
index 0000000..b7b09f1
--- /dev/null
+++ b/tests/nec-usb-xhci-test.c
@@ -0,0 +1,102 @@
+/*
+ * QTest testcase for nec-usb-xhci crash on writeb to bar 0
+ *
+ * Copyright (c) 2015 Salva Peiró <speiro.fr@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+/*
+    Steps to reproduce the scenario:
+
+    1) Compile with:
+    make tests/nec-usb-xhci-test 
+
+    2) Run with:
+    QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386 tests/nec-usb-xhci-test 
+    
+    3) Output:
+    /i386/nec-usb-xhci/writeb: 
+    dev 1234:1111 bar 2 [0xe0000000/4096]
+    dev 1033:194 bar 0 [0xe0001000/16384]
+    Broken pipe
+
+    Program received signal SIGSEGV, Segmentation fault.
+    0x0000000000000000 in ?? ()
+    (db) bt
+    #0  0x0000000000000000 in ?? ()
+    #1  0x00007f4f2651af83 in memory_region_oldmmio_write_accessor (attrs=..., mask=<optimized out>, shift=0, size=<optimized out>, value=<synthetic pointer>, addr=3, 
+            mr=<optimized out>) at /n/m/r/qemu.git/memory.c:450
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <glib.h>
+#include <string.h>
+#include "qemu/osdep.h"
+
+#include "libqtest.h"
+#include "libqos/pci.h"
+#include "libqos/pci-pc.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+    QPCIDevice **pdev = (QPCIDevice **) data;
+    *pdev = dev;
+}
+
+static void test_device(gconstpointer data)
+{
+    QTestState *s;
+
+    QPCIBus *pcibus;
+    QPCIDevice *dev;
+    uintptr_t *bar;
+    uint64_t  barsize;
+    int vendor_id, device_id, b=0;
+
+    printf("\n");
+    s = qtest_start("-M q35 -device nec-usb-xhci");
+    g_assert(s != NULL);
+
+    pcibus = qpci_init_pc();
+
+    vendor_id=0x1234; device_id=0x1111; b=2;
+    qpci_device_foreach(pcibus, vendor_id, device_id, save_fn, &dev);
+    g_assert(dev != NULL);
+
+    qpci_device_enable(dev);
+    bar = (uintptr_t*) qpci_iomap(dev, b, &barsize);
+    printf("dev %02x:%02x bar %d [%p/%d]\n", vendor_id, device_id, b, bar, (int)barsize);
+    qpci_io_writeb(dev, (void*)bar, 0x0);
+
+    vendor_id=0x1033; device_id=0x0194; b=0;
+    qpci_device_foreach(pcibus, vendor_id, device_id, save_fn, &dev);
+    g_assert(dev != NULL);
+    
+    qpci_device_enable(dev);
+    bar = (uintptr_t*) qpci_iomap(dev, b, &barsize);
+    printf("dev %02x:%02x bar %d [%p/%d]\n", vendor_id, device_id, b, bar, (int)barsize);
+    uintptr_t addr = (uintptr_t)bar + 0xe803 % barsize;
+    qpci_io_writeb(dev, (void*)addr, 0x0);
+
+    if (s) {
+        qtest_quit(s);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    char *path;
+
+    g_test_init(&argc, &argv, NULL);
+
+    path = g_strdup_printf("nec-usb-xhci/writeb");
+    qtest_add_data_func(path, NULL, test_device);
+
+    return g_test_run();
+}
-- 
2.1.4

Re: [Qemu-devel] [PATCH] memory: Add function pointers checks to memory_region_read/write()

[Paolo Bonzini]

On 08/09/2015 08:51, Salva Peiró wrote:
> Rigth, I should have started by providing the scenario where the fault occurs.
> The problem occurs performing a writeb to the BAR0 of device 1033:194.
> That is PCI_DEVICE_ID_NEC_UPD720200  0x0194 at hw/usb/hcd-xhci.c
> 
> I've attached tests/nec-usb-xhci-test.c that reproduces the scenario.

Thanks, this is a good addition.  Please submit a fix to hcd-xhci.c and
the test case!

Paolo