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

[Laszlo Ersek]

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.

Thanks
Laszlo