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

[Alex Williamson]

On Tue, 25 Feb 2020 07:48:33 +0800
"Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
<address@hidden> wrote:

> On 2020/2/25 0:04, Alex Williamson wrote:
> > On Mon, 24 Feb 2020 14:42:17 +0800
> > "Longpeng(Mike)" <address@hidden> wrote:
> >   
> >> From: Longpeng <address@hidden>
> >>
> >> vfio_pci_load_rom() maybe failed and then the vdev->rom is NULL in
> >> some situation (though I've not encountered yet), maybe we should
> >> avoid the VM abort.
> >>
> >> Signed-off-by: Longpeng <address@hidden>
> >> ---
> >>  hw/vfio/pci.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 5e75a95..ed798ae 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -768,7 +768,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
> >>      }
> >>  }
> >>  
> >> -static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> >> +static bool vfio_pci_load_rom(VFIOPCIDevice *vdev)
> >>  {
> >>      struct vfio_region_info *reg_info;
> >>      uint64_t size;
> >> @@ -778,7 +778,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> >>      if (vfio_get_region_info(&vdev->vbasedev,
> >>                               VFIO_PCI_ROM_REGION_INDEX, &reg_info)) {
> >>          error_report("vfio: Error getting ROM info: %m");
> >> -        return;
> >> +        return false;
> >>      }
> >>  
> >>      trace_vfio_pci_load_rom(vdev->vbasedev.name, (unsigned 
> >> long)reg_info->size,
> >> @@ -797,7 +797,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> >>          error_printf("Device option ROM contents are probably invalid "
> >>                      "(check dmesg).\nSkip option ROM probe with rombar=0, 
> >> "
> >>                      "or load from file with romfile=\n");
> >> -        return;
> >> +        return false;
> >>      }
> >>  
> >>      vdev->rom = g_malloc(size);
> >> @@ -849,6 +849,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> >>              data[6] = -csum;
> >>          }
> >>      }
> >> +
> >> +    return true;
> >>  }
> >>  
> >>  static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
> >> @@ -863,8 +865,9 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr 
> >> addr, unsigned size)
> >>      uint64_t data = 0;
> >>  
> >>      /* Load the ROM lazily when the guest tries to read it */
> >> -    if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
> >> -        vfio_pci_load_rom(vdev);
> >> +    if (unlikely(!vdev->rom && !vdev->rom_read_failed) &&
> >> +        !vfio_pci_load_rom(vdev)) {
> >> +        return 0;
> >>      }
> >>  
> >>      memcpy(&val, vdev->rom + addr,  
> > 
> > Looks like an obvious bug, until you look at the rest of this memcpy():
> > 
> > memcpy(&val, vdev->rom + addr,
> >            (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
> > 
> > IOW, we'll do a zero sized memcpy() if rom_size is zero, so there's no
> > risk of the concern identified in the commit log.  This patch is
> > unnecessary.  Thanks,
> >   
> Oh, I missed that, sorry for make the noise, thanks

Actually, not noise.  After some internal discussion thanks to Laszlo,
it seems that while memcpy() with a zero size seems to do the right
thing, the behavior for any case where we pass a null pointer is not
actually defined.  However, there's still a bug in the implementation
of the fix above, if vdev->rom_read_failed is set, we'll still fall
through to the memcpy.  I think there's also another bug in the current
implementation that we initialize data to zero but we'll overwrite it
with the uninitialized 'val' in the switch statement.  I think the
below resolves both.  I'll formally post it after a bit of testing:

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: