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

[Markus Armbruster <armbru@redhat.com>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/11/20 02:36, Alex Williamson wrote:
>> On Wed, 11 Mar 2020 00:14:31 +0100
>> Laszlo Ersek <lersek@redhat.com> wrote:
>> 
>>> On 03/10/20 17:11, Alex Williamson wrote:
>>>
>>>> commit 2088fc1e1f426b98e9ca4d7bcdbe53d886a18c37
>>>> Author: Alex Williamson <alex.williamson@redhat.com>
>>>> 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 <longpeng2@huawei.com>
>>>>     Reported-by: Laszlo Ersek <lersek@redhat.com>
>>>>     Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>
>>>> 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! :)

You're looking for

'-Wpointer-arith'
     Warn about anything that depends on the "size of" a function type
     or of 'void'.  GNU C assigns these types a size of 1, for
     convenience in calculations with 'void *' pointers and pointers to
     functions.  In C++, warn also when an arithmetic operation involves
     'NULL'.  This warning is also enabled by '-Wpedantic'.