[libvirt] [PATCH] util: Prevent libvirtd crash from virPCIDeviceAddressIsEqual()

[Laine Stump]

On 05/02/2013 02:06 AM, Alex Jia wrote:
> On 04/29/2013 02:48 AM, Laine Stump wrote:
>> On 04/28/2013 06:12 AM, Alex Jia wrote:
>>> GDB backtrace:
>>>
>>> Breakpoint 1, virPCIGetVirtualFunctionIndex
>>> (pf_sysfs_device_link=0x7fc04400f470
>>> "/sys/bus/pci/devices/0000:03:00.1", vf_sysfs_device_link=<optimized
>>> out>, vf_index=vf_index at entry=0x7fc06897b8f4)
>>>      at util/virpci.c:2107
>>> 2107            if (virPCIDeviceAddressIsEqual(vf_bdf, virt_fns[i])) {
>>> (gdb) p *vf_bdf
>>> $1 = {domain = 0, bus = 3, slot = 16, function = 1}
>>> (gdb) l
>>> 2102                             "virtual_functions"),
>>> pf_sysfs_device_link);
>>> 2103            goto out;
>>> 2104        }
>>> 2105
>>> 2106        for (i = 0; i<  num_virt_fns; i++) {
>>> 2107            if (virPCIDeviceAddressIsEqual(vf_bdf, virt_fns[i])) {
>>> 2108                *vf_index = i;
>>> 2109                ret = 0;
>>> 2110                break;
>>> 2111            }
>>> (gdb) p num_virt_fns
>>> $46 = 2
>>> (gdb) p virt_fns[0]
>>> $48 = (virPCIDeviceAddressPtr) 0x0
>>> (gdb) s
>>> virPCIDeviceAddressIsEqual (bdf2=0x0, bdf1=0x7fc04400f330) at
>>> util/virpci.c:1844
>>> 1844                (bdf1->slot == bdf2->slot)&&
>>> (gdb) s
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>>
>>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=957416
>>>
>>> Signed-off-by: Alex Jia<ajia at redhat.com>
>>> ---
>>>   src/util/virpci.c |    3 ++-
>>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>>> index 97bba74..dda044c 100644
>>> --- a/src/util/virpci.c
>>> +++ b/src/util/virpci.c
>>> @@ -1897,7 +1897,8 @@ static bool
>>>   virPCIDeviceAddressIsEqual(virPCIDeviceAddressPtr bdf1,
>>>                              virPCIDeviceAddressPtr bdf2)
>>>   {
>>> -    return ((bdf1->domain == bdf2->domain)&&
>>> +    return (bdf1&&  bdf2&&
>>> +            (bdf1->domain == bdf2->domain)&&
>>>               (bdf1->bus == bdf2->bus)&&
>>>               (bdf1->slot == bdf2->slot)&&
>>>               (bdf1->function == bdf2->function));
>> NACK.
>>
>> This patch only fixes the symptom (not the root cause), and only in the
>> case of starting a domain with an<interface type='hostdev'. It doesn't
>> fix the second crash described in the BZ when running virsh
>> nodedev-dumpxml - the code path of that doesn't ever get to
>
> Yes, I just noticed this, it should be different code path.
>
>> virPCIDeviceAddressIsEqual() (but *does* call the function that actually
>> has the bug).
>
> Yes, another bug.


IMO, the only bug here.


>
>> The root cause of these crashes was a typo introduced just before the
>> release of 1.0.4. I found that problem and pushed the correct patch on
>> April 9:
>>
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=9579b6bc209b46a0f079b21455b598c817925b48
>>
>
> It should be okay if the patch is backported into rhel.


No backport is needed. This bug was only in one release of libvirt, and
that release is not (and almost surely will not be) in any public RHEL
or Fedora release.


>> (Beyond that, I don't like the idea of ignoring a NULL pointer -
>> virPCIDeviceAddressIsEqual should always be passed non-NULL pointers,
>> and its only current caller does guarantee that (except for when it has
>> a bug). If we want virPCIDeviceAddressIsEqual to do something with NULL
>> pointers, it should be logging an error and failing, but that would
>> complicate the interface to the function beyond just returning a
>> true/false (it would have to be tri-state, and the caller would need to
>
> Yes, the function interface will be more complex and return value should
> be tri-state not simple true/false.


Which will make the code in the caller unnecessarily more complex, since
we've already checked for a valid pointer and handled it earlier in that
same function (or to be more exact, a function called by that one -
virPCIGetVirtualFunctions(), which does guarantee that only non-NULL
pointers are placed into the array (when written correctly, as it is now
after the bugfix patch above).


>
>> check all three possibilities). I think in this case it's better for the
>> caller to make sure the pointers it sends are valid.)
>
> Yes, a simple way is the caller makes sure pointers is Non-NULL, maybe,
> we need also to add comment "The caller should make sure the pointers
> it sends are valid" into the virPCIDeviceAddressIsEqual().


That's kind of a given for *any* function :-)


> However,
> in order to avoid the caller missing argument judgement, I think we
> should also fix().

There are many private functions in libvirt that assume the caller has
sent in sane arguments. If every function checked every argument for
non-NULL, the code would be full of that. I think that's only necessary
for API-level functions, but this function is only a static, used just
within the same file (and only once).

I think that the proper course of action here is to leave it as it is;
note that if it had been "fixed" at the time the bug was introduced to
virPCIGetVirtualFunctions(), it would have been *much* more difficult to
detect, identify, and fix that bug.