Skip to contentSkip to conversation

exec: Fix bounce buffer allocation in address_space_map()

PATCH Superseded Kevin Wolf 2 participants 5 messages Jul 22, 2013

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

cover exec: Fix bounce buffer allocation in address_space_map()

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(-)
Signature · 1 line
1.8.1.4
author
#1 lore raw

PATCH 1/2 exec: Fix bounce buffer allocation in address_space_map()

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 <[email protected]>
---
 exec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
exec.c +3 -1
index c99a883..53cbbdf 100644
@@ -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;
Signature · 1 line
1.8.1.4
#2 lore raw

Il 22/07/2013 14:43, Kevin Wolf ha scritto:

exec.c void *address_space_map(AddressSpace *as, · line 2169

Quoted 3 lines
exec.c +2 -1
-        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

Quoted 1 line

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

author
#3 lore raw

Am 22.07.2013 um 15:06 hat Paolo Bonzini geschrieben:

Quoted 6 lines

Il 22/07/2013 14:43, Kevin Wolf ha scritto:

exec.c void *address_space_map(AddressSpace *as, · line 2169

Quoted 3 lines
exec.c +2 -1
-        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

author
#4 lore raw

PATCH 2/2 ide-test: Check what happens with bus mastering disabled

In reply to Kevin Wolf

The main goal is that qemu doesn't crash.

Signed-off-by Kevin Wolf <[email protected]>
---
 tests/ide-test.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
tests/ide-test.c +26 -0
index 7307f1d..bc824a8 100644
@@ -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);
Signature · 1 line
1.8.1.4