Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read

[Laszlo Ersek]

On 03/11/20 02:36, Alex Williamson wrote:
> On Wed, 11 Mar 2020 00:14:31 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 03/10/20 17:11, Alex Williamson wrote:
>>
>>> commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37
>>> Author: Alex Williamson <address@hidden>
>>> Date:   Tue Mar 10 10:04:36 2020 -0600
>>>
>>>     vfio/pci: Use defined memcpy() behavior
>>>     
>>>     vfio_rom_read() relies on memcpy() doing the logically correct thing,
>>>     ie. safely copying zero bytes from a NULL pointer when rom_size is
>>>     zero, rather than the spec definition, which is undefined when the
>>>     source or target pointers are NULL.  Resolve this by wrapping the
>>>     call in the condition expressed previously by the ternary.
>>>     
>>>     Additionally, we still use @val to fill data based on the provided
>>>     @size regardless of mempcy(), so we should initialize @val rather
>>>     than @data.
>>>     
>>>     Reported-by: Longpeng <address@hidden>
>>>     Reported-by: Laszlo Ersek <address@hidden>
>>>     Signed-off-by: Alex Williamson <address@hidden>
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 5e75a95129ac..b0799cdc28ad 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -859,16 +859,17 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr 
>>> addr, unsigned size)
>>>          uint16_t word;
>>>          uint32_t dword;
>>>          uint64_t qword;
>>> -    } val;
>>> -    uint64_t data = 0;
>>> +    } val = { 0 };
>>> +    uint64_t data;
>>>  
>>>      /* Load the ROM lazily when the guest tries to read it */
>>>      if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
>>>          vfio_pci_load_rom(vdev);
>>>      }
>>>  
>>> -    memcpy(&val, vdev->rom + addr,
>>> -           (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
>>> +    if (addr < vdev->rom_size) {
>>> +        memcpy(&val, vdev->rom + addr, MIN(size, vdev->rom_size - addr));
>>> +    }
>>>  
>>>      switch (size) {
>>>      case 1:  
>>
>> Regarding the pre-patch code:
>>
>> My understanding is that the memcpy() could be reached with a
>> guest-originated "addr" even if "vdev->rom" was NULL. If that's the
>> case, then the pre-patch code invokes undefined behavior regardless of
>> memcpy(), because it performs pointer arithmetic on a null pointer (not
>> to mention that the type of that pointer is (void *)....)
>>
>> Regarding the proposed change:
>>
>> (addr < vdev->rom_size) requires that "vdev->rom_size" be positive. In
>> that case, I assume that
>>
>> - "vdev->rom" is not NULL, and
>> -  MIN(size, vdev->rom_size - addr) bytes are "in range" for the object
>> allocated at "vdev->rom".
>>
>> So from a memcpy() and range perspective, the patch looks OK. But
>> there's still a wart I dislike: we should never perform pointer
>> arithmetic on a (void*). I suggest casting (vdev->rom) to (uint8_t*) or
>> (unsigned char*) first.
>>
>> Here's an excerpt from the ISO C99 standard:
>>
>> -v-
>> 6.5.6 Additive operators
>>
>> Constraints
>>
>> 2 For addition, either both operands shall have arithmetic type, or one
>>   operand shall be a pointer to an object type and the other shall have
>>   integer type. [...]
>> -^-
>>
>> A "pointer-to-void" is not a "pointer to an object type", because "void"
>> is not an object type -- it is an incomplete type that cannot be completed:
>>
>> -v-
>> 6.2.5 Types
>>
>> 1 [...] Types are partitioned into object types (types that fully
>>   describe objects), function types (types that describe functions), and
>>   incomplete types (types that describe objects but lack information
>>   needed to determine their sizes).
>>
>> [...]
>>
>> 19 The void type comprises an empty set of values; it is an incomplete
>>    type that cannot be completed.
>> -^-
>>
>> For a different illustration, (vdev->rom + addr) is equivalent to
>> &(vdev->rom[addr]) -- and we clearly can't have an "array of void".
>>
>> This anti-pattern (of doing pointer arithmetic on (void*)) likely comes
>> from a guarantee that the standard does make, in the same "6.2.5 Types"
>> section:
>>
>> -v-
>> 27 A pointer to void shall have the same representation and alignment
>>    requirements as a pointer to a character type. 39) [...]
>>
>> Footnote 39: The same representation and alignment requirements are
>>              meant to imply interchangeability as arguments to
>>              functions, return values from functions, and members of
>>              unions.
>> -^-
>>
>> It does not extend to the "+" operator.
> 
> GNU C specifically allows arithmetic on pointers and defines the size
> of a void as 1.  I'll comply, but this makes me want to stab myself in
> the face :-\  Thanks,

Sorry, I didn't want to annoy you. :)

In fact I was about to mention, "I really don't understand why compilers
don't yell upon seeing pointer-to-void arithmetic", but I got distracted
and forgot about that thought. In retrospect, that may have been for the
best! :)

Thanks
Laszlo