vfio-pci: fix assert fail in host property if unused
cover vfio-pci: fix assert fail in host property if unused
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 when get default PCI address property used by vfio-pci
hw/core/qdev-properties.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
Signature · 1 line
2.10.2
PATCH 1/1 Fix assert when get default PCI address property used by vfio-pci
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.
--- hw/core/qdev-properties.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
hw/core/qdev-properties.c +13 -5
@@ -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); }
Signature · 1 line
2.10.2
Re: [Qemu-devel] [PATCH 1/1] Fix assert when get default PCI address property used by vfio-pci
Quoted 2 lines · On Wed, 9 Nov 2016 18:36:20 +0000 Daniel Oram <[email protected]> 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.
nit, wrap your commit log at ~70 chars
Quoted 26 lines
Signed-off-by Daniel Oram <[email protected]>--- hw/core/qdev-properties.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)hw/core/qdev-properties.c +6
-5@@ -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. +*/
hw/core/qdev-properties.c static void get_pci_host_devaddr(Object *obj, Visitor *v, const char *name, · line 725
Quoted 10 lines
hw/core/qdev-properties.c +7
-0+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); }
Works for me, note that slot and function are actually 5 bits and 3 bits respectively, so an address ending in "ff.f" can easily be recognized as invalid to the user, the highest actual address would be "ffff:ff:1f.7".
Can anyone take this for 2.8? Thanks,
Alex