[Qemu-devel] [PATCH v2 0/1] vfio-pci: fix assert fail in host property if unused

[Qemu-devel] [PATCH v2 0/1] vfio-pci: fix assert fail in host property if unused

[Daniel Oram]

Changes since v1:
* Wrap commit log at 70 chars.

Commit 4a946268 changed the default value of the structure (PCIHostDeviceAddress) underlying the host property in vfio-pci to be ~0 in all fields. Since this structure has excess bits for representing a standard BDF (FFFF:FF:FF.F) this triggers an assert check designed to catch such invalid BDFs in the get function of the property. This makes any code that attempts to use get on the property fatal if the host device isn't specified using the now optional host property.

To see the bug assign a vfio-pci device using the sysfsdev property instead of the host property so that host gets the default "not present," value. Attempts to display the property then crash the working emulation.

qemu-system-x86_64 -device vfio-pci,id=gfxfn0,sysfsdev='/sys/bus/pci/devices/0000:01:00.0' -monitor stdio

QEMU 2.7.50 monitor - type 'help' for more information
(qemu) info qtree
bus: main-system-bus
....Omitted for brevity...
    bus: pci.0
      type PCI
      dev: vfio-pci, id "gfxfn0"
qemu-system-x86_64: /home/xochip/source/qemu.git/hw/core/qdev-properties.c:717: get_pci_host_devaddr: Assertion `rc == sizeof(buffer) - 1' failed.

The bug is minor because the structure involved is presumably insufficient and redundant given the introduction of the new sysfsdev property. Since I'm new to the code, I resisted the urge to make a mess by cleaning it up and attach a totally minimal fix in the hope it makes the problem clearer and easier to ignore. Happy to redo or leave it to somebody else as required.

Regards,

Dan.

Daniel Oram (1):
  Fix assert in PCI address property when used by vfio-pci

 hw/core/qdev-properties.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.10.2

[Qemu-devel] [PATCH v2 1/1] Fix assert in PCI address property when used by vfio-pci

[Daniel Oram]

Allow the PCIHostDeviceAddress structure to work as the host property
in vfio-pci when it has it's default value of all fields set to ~0. In
this form the property indicates a non-existant device but given the
field bit sizes gets asserted as excess (and invalid) precision
overflows the string buffer. The BDF of an invalid device
"FFFF:FF:FF.F" is returned instead.

Signed-off-by: Daniel Oram <daniel.oram@gmail.com>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

---
v2:
  - Wrap commit log at 70 chars.
---
 hw/core/qdev-properties.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 311af6d..ed0d5b0 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -705,13 +705,21 @@ static void get_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
     PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
-    char buffer[] = "xxxx:xx:xx.x";
+    char buffer[] = "ffff:ff:ff.f";
     char *p = buffer;
     int rc = 0;
-
-    rc = snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%d",
-                  addr->domain, addr->bus, addr->slot, addr->function);
-    assert(rc == sizeof(buffer) - 1);
+    
+    /*
+     * Catch "invalid" device reference from vfio-pci and allow the 
+     * default buffer representing the non-existant device to be used.
+     */
+    if (~addr->domain || ~addr->bus || ~addr->slot || ~addr->function) {
+           
+        rc = snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%0d",
+                      addr->domain, addr->bus, addr->slot, addr->function);
+        assert(rc == sizeof(buffer) - 1);
+    }
+    
 
     visit_type_str(v, name, &p, errp);
 }
-- 
2.10.2

Re: [Qemu-devel] [PATCH v2 0/1] vfio-pci: fix assert fail in host property if unused

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v2 0/1] vfio-pci: fix assert fail in host property if unused
Message-id: cover.1478777270.git.daniel.oram@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
1aec960 Fix assert in PCI address property when used by vfio-pci

=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/1: ...
ERROR: trailing whitespace
#34: FILE: hw/core/qdev-properties.c:711:
+    $

ERROR: trailing whitespace
#36: FILE: hw/core/qdev-properties.c:713:
+     * Catch "invalid" device reference from vfio-pci and allow the $

ERROR: trailing whitespace
#40: FILE: hw/core/qdev-properties.c:717:
+           $

ERROR: trailing whitespace
#45: FILE: hw/core/qdev-properties.c:722:
+    $

total: 4 errors, 0 warnings, 26 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 1/1] Fix assert in PCI address property when used by vfio-pci

[Eduardo Habkost]

On Thu, Nov 10, 2016 at 12:42:07PM +0000, Daniel Oram wrote:
> Allow the PCIHostDeviceAddress structure to work as the host property
> in vfio-pci when it has it's default value of all fields set to ~0. In
> this form the property indicates a non-existant device but given the
> field bit sizes gets asserted as excess (and invalid) precision
> overflows the string buffer. The BDF of an invalid device
> "FFFF:FF:FF.F" is returned instead.
> 
> Signed-off-by: Daniel Oram <daniel.oram@gmail.com>
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Applied to machine-next after removing trailing-spaces detected
by checkpatch.pl. Thanks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/1] Fix assert in PCI address property when used by vfio-pci

[Alex Williamson]

On Tue, 15 Nov 2016 16:01:52 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Nov 10, 2016 at 12:42:07PM +0000, Daniel Oram wrote:
> > Allow the PCIHostDeviceAddress structure to work as the host property
> > in vfio-pci when it has it's default value of all fields set to ~0. In
> > this form the property indicates a non-existant device but given the
> > field bit sizes gets asserted as excess (and invalid) precision
> > overflows the string buffer. The BDF of an invalid device
> > "FFFF:FF:FF.F" is returned instead.
> > 
> > Signed-off-by: Daniel Oram <daniel.oram@gmail.com>
> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com>  
> 
> Applied to machine-next after removing trailing-spaces detected
> by checkpatch.pl. Thanks.

Thanks for grabbing this, Eduardo.

Alex