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

["Salva Peiró" <speirofr@gmail.com>]

[-- 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