[libvirt] [PATCH 0/2] Fix PCI address validation
Pavel Hrdina (2): domain_conf: refactor virDomainDeviceAddressIsValid to boolean device_conf: PCI address pci_0000_00_00_0 is also valid src/conf/device_conf.c | 7 ++++--- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 16 ++++++++-------- src/conf/domain_conf.h | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-) -- 1.8.5.5
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/device_conf.c | 4 ++-- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 16 ++++++++-------- src/conf/domain_conf.h | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index b3b04e1..61c73dc 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -39,11 +39,11 @@ VIR_ENUM_IMPL(virInterfaceState, "down", "lowerlayerdown", "testing", "dormant", "up") -int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) +bool virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { /* PCI bus has 32 slots and 8 functions per slot */ if (addr->slot >= 32 || addr->function >= 8) - return 0; + return false; return addr->domain || addr->bus || addr->slot; } diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f067a35..1808c26 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -62,7 +62,7 @@ struct _virInterfaceLink { unsigned int speed; /* link speed in Mbits per second */ }; -int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); +bool virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); int virDevicePCIAddressParseXML(xmlNodePtr node, virDevicePCIAddressPtr addr); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c25c74b..712ba37 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2493,7 +2493,7 @@ void virDomainObjListRemoveLocked(virDomainObjListPtr doms, virHashRemoveEntry(doms->objs, uuidstr); } -static int +static bool virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr) { return addr->cssid <= VIR_DOMAIN_DEVICE_CCW_MAX_CSSID && @@ -2501,31 +2501,31 @@ virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr) addr->devno <= VIR_DOMAIN_DEVICE_CCW_MAX_DEVNO; } -int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, - int type) +bool virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, + int type) { if (info->type != type) - return 0; + return false; switch (info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: return virDevicePCIAddressIsValid(&info->addr.pci); case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: - return 1; + return true; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - return 1; + return true; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: return virDomainDeviceCCWAddressIsValid(&info->addr.ccw); case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: - return 1; + return true; } - return 0; + return false; } virDomainDeviceInfoPtr diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bffc0a5..d7c9bf8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2208,8 +2208,8 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, const virDomainDef *def, virCapsPtr caps, virDomainXMLOptionPtr xmlopt); -int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, - int type); +bool virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, + int type); virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src); -- 1.8.5.5
On 08/05/2014 10:18 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/device_conf.c | 4 ++-- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 16 ++++++++-------- src/conf/domain_conf.h | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
All the variables used for pci addr in virDevicePCIAddress are unsigned and therefore in this validation function they never cannot be less than zero. This check will fail only for the address above, but that address is valid so we can safely return true instead. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1055331 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/device_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 61c73dc..797b2b6 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -44,7 +44,8 @@ bool virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) /* PCI bus has 32 slots and 8 functions per slot */ if (addr->slot >= 32 || addr->function >= 8) return false; - return addr->domain || addr->bus || addr->slot; + + return true; } -- 1.8.5.5
On 08/05/2014 10:18 AM, Pavel Hrdina wrote:
All the variables used for pci addr in virDevicePCIAddress are unsigned and therefore in this validation function they never cannot be less than zero. This check will fail only for the address above, but that address is valid so we can safely return true instead.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1055331
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/device_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 61c73dc..797b2b6 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -44,7 +44,8 @@ bool virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) /* PCI bus has 32 slots and 8 functions per slot */ if (addr->slot >= 32 || addr->function >= 8) return false; - return addr->domain || addr->bus || addr->slot; + + return true; }
I think we're going to need more than this :( domain_conf.c uses it to feed this code in virDomainNetFindIdx: bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI); Right now, if you omit an <address> element, it is 0-initialized and the in-memory structure happens to be pci_0000_00_00_0, but PCIAddrSpecified is set to false, which then lets us auto-pick an appropriate (non-zero) address. If you explicitly specify an all-0 address, you get the same behavior. With your patch, it looks like you want to allow explicit specification of an all-0 address. But for that to work, we now need to modify domain_conf.h (and/or device_conf.h) to ALSO have a bool flag that says whether an address was specified (since you are allowing explicit specification of all-0, where we should NOT auto-assign a different address) or omitted. NACK to this version; the correct solution is one where an explicit all-0 address behaves differently than omitting <address>. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Pavel Hrdina