[Qemu-devel] [PATCH 0/2] exec: Fix bounce buffer allocation in address_space_map()

[Qemu-devel] [PATCH 0/2] exec: Fix bounce buffer allocation in address_space_map()

[Kevin Wolf]

Kevin Wolf (2):
  exec: Fix bounce buffer allocation in address_space_map()
  ide-test: Check what happens with bus mastering disabled

 exec.c           |  4 +++-
 tests/ide-test.c | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] exec: Fix bounce buffer allocation in address_space_map()

[Kevin Wolf]

This fixes a regression introduced by commit e3127ae0c, which kept the
allocation size of the bounce buffer limited to one page in order to
avoid unbounded allocations (as explained in the commit message of
6d16c2f88), but broke the reporting of the shortened bounce buffer to
the caller. The caller therefore assumes that the full requested size
was provided and causes memory corruption when writing beyond the end of
the actually allocated buffer.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 exec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index c99a883..53cbbdf 100644
--- a/exec.c
+++ b/exec.c
@@ -2165,7 +2165,9 @@ void *address_space_map(AddressSpace *as,
         if (bounce.buffer) {
             return NULL;
         }
-        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
+        /* Avoid unbounded allocations */
+        l = TARGET_PAGE_SIZE;
+        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
         bounce.addr = addr;
         bounce.len = l;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] exec: Fix bounce buffer allocation in address_space_map()

[Paolo Bonzini]

Il 22/07/2013 14:43, Kevin Wolf ha scritto:
> -        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
> +        /* Avoid unbounded allocations */
> +        l = TARGET_PAGE_SIZE;

This should be l = MIN(l, TARGET_PAGE_SIZE).  Otherwise the patch is okay.

Paolo

> +        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);

Re: [Qemu-devel] [PATCH 1/2] exec: Fix bounce buffer allocation in address_space_map()

[Kevin Wolf]

Am 22.07.2013 um 15:06 hat Paolo Bonzini geschrieben:
> Il 22/07/2013 14:43, Kevin Wolf ha scritto:
> > -        bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
> > +        /* Avoid unbounded allocations */
> > +        l = TARGET_PAGE_SIZE;
> 
> This should be l = MIN(l, TARGET_PAGE_SIZE).  Otherwise the patch is okay.

Heh, okay. That's what I first had, but then I changed it to revert to
the original behaviour, because I wasn't sure if qemu_memalign requires
the size to be aligned as well.

I'll send a v2.

Kevin

[Qemu-devel] [PATCH 2/2] ide-test: Check what happens with bus mastering disabled

[Kevin Wolf]

The main goal is that qemu doesn't crash.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/ide-test.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 7307f1d..bc824a8 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -81,6 +81,7 @@ enum {
     CMD_IDENTIFY    = 0xec,
 
     CMDF_ABORT      = 0x100,
+    CMDF_NO_BM      = 0x200,
 };
 
 enum {
@@ -192,6 +193,11 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
         g_assert_not_reached();
     }
 
+    if (flags & CMDF_NO_BM) {
+        qpci_config_writew(dev, PCI_COMMAND,
+                           PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
+    }
+
     /* Select device 0 */
     outb(IDE_BASE + reg_device, 0 | LBA);
 
@@ -352,6 +358,25 @@ static void test_bmdma_long_prdt(void)
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 }
 
+static void test_bmdma_no_busmaster(void)
+{
+    uint8_t status;
+
+    /* No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't be
+     * able to access it anyway because the Bus Master bit in the PCI command
+     * register isn't set. This is complete nonsense, but it used to be pretty
+     * good at confusing and occasionally crashing qemu. */
+    PrdtEntry prdt[4096] = { };
+
+    status = send_dma_request(CMD_READ_DMA | CMDF_NO_BM, 0, 512,
+                              prdt, ARRAY_SIZE(prdt));
+
+    /* Not entirely clear what the expected result is, but this is what we get
+     * in practice. At least we want to be aware of any changes. */
+    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
+    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+}
+
 static void test_bmdma_setup(void)
 {
     ide_test_start(
@@ -493,6 +518,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/bmdma/simple_rw", test_bmdma_simple_rw);
     qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
     qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
+    qtest_add_func("/ide/bmdma/no_busmaster", test_bmdma_no_busmaster);
     qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
 
     qtest_add_func("/ide/flush", test_flush);
-- 
1.8.1.4