Skip to contentSkip to conversation

exec: Fix bounce buffer allocation in address_space_map()

PATCH New Kevin Wolf 1 participant 3 messages Jul 22, 2013

lore
Share & tools
lore

Kevin Wolf

author
#0 lore raw

v2 cover exec: Fix bounce buffer allocation in address_space_map()

v2:
- l = MIN(l, TARGET_PAGE_SIZE) instead of unconditionally TARGET_PAGE_SIZE

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 v2 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..26aa9e8 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 = MIN(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
author
#2 lore raw

PATCH v2 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