[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_ro
From: |
Alex Williamson |
Subject: |
Re: [PATCH RESEND 1/3] vfio/pci: fix a null pointer reference in vfio_rom_read |
Date: |
Tue, 10 Mar 2020 19:36:24 -0600 |
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,
Alex