Re: [PATCH] vfio/pci: Use defined memcpy() behavior

[Alex Williamson]

On Wed, 11 Mar 2020 10:54:12 +0800
"Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
<address@hidden> wrote:

> On 2020/3/11 1:15, Alex Williamson wrote:
> > 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>
> > ---
> >  hw/vfio/pci.c |    9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > 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;
> >    
> 'val.qword' won't be used, maybe we could drop it silently.

Or we could specify that we support 8-byte accesses and expand the
switch to include it.  Neither is terribly pressing at this point, so
I'll leave that for some future work.
 
> Reviewed-by: Longpeng <address@hidden>

Thanks!

Alex
 
> >      /* 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:
> > 
> > .
> >   
> ---
> Regards,
> Longpeng(Mike)
>