[PATCH RESEND 0/3] fix some warnings by static code scan tool

[PATCH RESEND 0/3] fix some warnings by static code scan tool

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

Hi guys,
Our tool find some potential issues in QEMU source code, maybe
they're misreported, hope you could have a look at them.

Longpeng (Mike) (3):
  vfio/pci: fix a null pointer reference in vfio_rom_read
  vhost: fix a null pointer reference of vhost_log
  util/pty: fix a null pointer reference in qemu_openpty_raw

 hw/vfio/pci.c       | 13 ++++++++-----
 hw/virtio/vhost.c   | 19 +++++++++++++++++--
 util/qemu-openpty.c | 10 ++--------
 3 files changed, 27 insertions(+), 15 deletions(-)

-- 
1.8.3.1

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

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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 <longpeng2@huawei.com>
---
 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,
-- 
1.8.3.1

[PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

vhost_log_alloc() may fails and returned pointer of log is null.
However there're two places derefernce the return pointer without
check.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vhost.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9edfadc..c7ad6e5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
 
     if (!log || log->size != size) {
         log = vhost_log_alloc(size, share);
+        if (!log) {
+            return NULL;
+        }
+
         if (share) {
             vhost_log_shm = log;
         } else {
@@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
 
 static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
 {
-    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
-    uint64_t log_base = (uintptr_t)log->log;
+    struct vhost_log *log;
+    uint64_t log_base;
     int r;
 
+    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
+    if (!log) {
+        return;
+    }
+
+    log_base = (uintptr_t)log->log;
+
     /* inform backend of log switching, this must be done before
        releasing the current log, to ensure no logging is lost */
     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
@@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         hdev->log_size = vhost_get_log_size(hdev);
         hdev->log = vhost_log_get(hdev->log_size,
                                   vhost_dev_log_is_shared(hdev));
+        if (!hdev->log) {
+            goto fail_vq;
+        }
+
         log_base = (uintptr_t)hdev->log->log;
         r = hdev->vhost_ops->vhost_set_log_base(hdev,
                                                 hdev->log_size ? log_base : 0,
-- 
1.8.3.1

[PATCH RESEND 3/3] util/pty: fix a null pointer reference in qemu_openpty_raw

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

q_ptsname may failed ane return null, so use the returned pointer
as the param of strcpy will cause null pointer deference. Use the
return string of openpty instead of call ptsname.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 util/qemu-openpty.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/util/qemu-openpty.c b/util/qemu-openpty.c
index 2e8b43b..2bea4ba 100644
--- a/util/qemu-openpty.c
+++ b/util/qemu-openpty.c
@@ -112,13 +112,7 @@ int qemu_openpty_raw(int *aslave, char *pty_name)
 {
     int amaster;
     struct termios tty;
-#if defined(__OpenBSD__) || defined(__DragonFly__)
-    char pty_buf[PATH_MAX];
-#define q_ptsname(x) pty_buf
-#else
-    char *pty_buf = NULL;
-#define q_ptsname(x) ptsname(x)
-#endif
+    char pty_buf[PATH_MAX] = { 0 };
 
     if (openpty(&amaster, aslave, pty_buf, NULL, NULL) < 0) {
         return -1;
@@ -130,7 +124,7 @@ int qemu_openpty_raw(int *aslave, char *pty_name)
     tcsetattr(*aslave, TCSAFLUSH, &tty);
 
     if (pty_name) {
-        strcpy(pty_name, q_ptsname(amaster));
+        strcpy(pty_name, pty_buf);
     }
 
     return amaster;
-- 
1.8.3.1

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

[Alex Williamson]

On Mon, 24 Feb 2020 14:42:17 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> From: Longpeng <longpeng2@huawei.com>
> 
> 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 <longpeng2@huawei.com>
> ---
>  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,

Alex

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

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

On 2020/2/25 0:04, Alex Williamson wrote:
> On Mon, 24 Feb 2020 14:42:17 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> 
>> From: Longpeng <longpeng2@huawei.com>
>>
>> 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 <longpeng2@huawei.com>
>> ---
>>  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

> Alex
> 
> .
> 

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

Hi Michael,

ping...

On 2020/2/24 14:42, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> vhost_log_alloc() may fails and returned pointer of log is null.
> However there're two places derefernce the return pointer without
> check.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>  hw/virtio/vhost.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9edfadc..c7ad6e5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>  
>      if (!log || log->size != size) {
>          log = vhost_log_alloc(size, share);
> +        if (!log) {
> +            return NULL;
> +        }
> +
>          if (share) {
>              vhost_log_shm = log;
>          } else {
> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>  
>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>  {
> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> -    uint64_t log_base = (uintptr_t)log->log;
> +    struct vhost_log *log;
> +    uint64_t log_base;
>      int r;
>  
> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> +    if (!log) {
> +        return;
> +    }
> +
> +    log_base = (uintptr_t)log->log;
> +
>      /* inform backend of log switching, this must be done before
>         releasing the current log, to ensure no logging is lost */
>      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          hdev->log_size = vhost_get_log_size(hdev);
>          hdev->log = vhost_log_get(hdev->log_size,
>                                    vhost_dev_log_is_shared(hdev));
> +        if (!hdev->log) {
> +            goto fail_vq;
> +        }
> +
>          log_base = (uintptr_t)hdev->log->log;
>          r = hdev->vhost_ops->vhost_set_log_base(hdev,
>                                                  hdev->log_size ? log_base : 0,
> 

-- 
---
Regards,
Longpeng(Mike)

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Michael S. Tsirkin]

On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> vhost_log_alloc() may fails and returned pointer of log is null.
> However there're two places derefernce the return pointer without
> check.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>  hw/virtio/vhost.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9edfadc..c7ad6e5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>  
>      if (!log || log->size != size) {
>          log = vhost_log_alloc(size, share);
> +        if (!log) {
> +            return NULL;
> +        }
> +
>          if (share) {
>              vhost_log_shm = log;
>          } else {
> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>  
>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>  {
> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> -    uint64_t log_base = (uintptr_t)log->log;
> +    struct vhost_log *log;
> +    uint64_t log_base;
>      int r;
>  
> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> +    if (!log) {
> +        return;
> +    }
> +

I'm not sure silently failing like this is safe. Callers assume
log can be resized. What can be done? I suspect not much
beside exiting ...
Speaking of which, lots of other failures in log resizing
path seem to be silently ignored.
I guess we should propagate them, and fix callers to check
the return code?

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
>> From: Longpeng <longpeng2@huawei.com>
>>
>> vhost_log_alloc() may fails and returned pointer of log is null.
>> However there're two places derefernce the return pointer without
>> check.
>>
>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> ---
>>  hw/virtio/vhost.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 9edfadc..c7ad6e5 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>>  
>>      if (!log || log->size != size) {
>>          log = vhost_log_alloc(size, share);
>> +        if (!log) {
>> +            return NULL;
>> +        }
>> +
>>          if (share) {
>>              vhost_log_shm = log;
>>          } else {
>> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>>  
>>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>>  {
>> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>> -    uint64_t log_base = (uintptr_t)log->log;
>> +    struct vhost_log *log;
>> +    uint64_t log_base;
>>      int r;
>>  
>> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>> +    if (!log) {
>> +        return;
>> +    }
>> +
> 
> I'm not sure silently failing like this is safe. Callers assume
> log can be resized. What can be done? I suspect not much
> beside exiting ...
> Speaking of which, lots of other failures in log resizing
> path seem to be silently ignored.
> I guess we should propagate them, and fix callers to check
> the return code?
> 
How about to let the callers treat the failure of log_resize as a fatal error ?

-static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
+static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
 {
-    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
-    uint64_t log_base = (uintptr_t)log->log;
+    struct vhost_log *log;
+    uint64_t log_base;
     int r;

+    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
+    if (!log) {
+        r = -1;
+        goto out;
+    }
+
+    log_base = (uintptr_t)log->log;
+
     /* inform backend of log switching, this must be done before
        releasing the current log, to ensure no logging is lost */
     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
@@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev
*dev, uint64_t size)
     vhost_log_put(dev, true);
     dev->log = log;
     dev->log_size = size;
+
+out:
+    return 0;
 }


@@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
 #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
     /* To log more, must increase log size before table update. */
     if (dev->log_size < log_size) {
-        vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
+        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
+            abort();
+        }
     }
     r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
     if (r < 0) {
@@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
     }
     /* To log less, can only decrease log size after table update. */
     if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
-        vhost_dev_log_resize(dev, log_size);
+        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
+            abort();
+        }
     }

 out:
@@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
int enable)
         }
         vhost_log_put(dev, false);
     } else {
-        vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+        r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+        if (r < 0) {
+            return r;
+        }
+
         r = vhost_dev_set_log(dev, true);
         if (r < 0) {
             return r;


> 
> .
> 

-- 
---
Regards,
Longpeng(Mike)

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Michael S. Tsirkin]

On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
> 
> 
> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> > On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> >> From: Longpeng <longpeng2@huawei.com>
> >>
> >> vhost_log_alloc() may fails and returned pointer of log is null.
> >> However there're two places derefernce the return pointer without
> >> check.
> >>
> >> Signed-off-by: Longpeng <longpeng2@huawei.com>
> >> ---
> >>  hw/virtio/vhost.c | 19 +++++++++++++++++--
> >>  1 file changed, 17 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 9edfadc..c7ad6e5 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
> >>  
> >>      if (!log || log->size != size) {
> >>          log = vhost_log_alloc(size, share);
> >> +        if (!log) {
> >> +            return NULL;
> >> +        }
> >> +
> >>          if (share) {
> >>              vhost_log_shm = log;
> >>          } else {
> >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> >>  
> >>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> >>  {
> >> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >> -    uint64_t log_base = (uintptr_t)log->log;
> >> +    struct vhost_log *log;
> >> +    uint64_t log_base;
> >>      int r;
> >>  
> >> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >> +    if (!log) {
> >> +        return;
> >> +    }
> >> +
> > 
> > I'm not sure silently failing like this is safe. Callers assume
> > log can be resized. What can be done? I suspect not much
> > beside exiting ...
> > Speaking of which, lots of other failures in log resizing
> > path seem to be silently ignored.
> > I guess we should propagate them, and fix callers to check
> > the return code?
> > 
> How about to let the callers treat the failure of log_resize as a fatal error ?
> 
> -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> +static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>  {
> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> -    uint64_t log_base = (uintptr_t)log->log;
> +    struct vhost_log *log;
> +    uint64_t log_base;
>      int r;
> 
> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> +    if (!log) {
> +        r = -1;
> +        goto out;
> +    }
> +
> +    log_base = (uintptr_t)log->log;
> +
>      /* inform backend of log switching, this must be done before
>         releasing the current log, to ensure no logging is lost */
>      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev
> *dev, uint64_t size)
>      vhost_log_put(dev, true);
>      dev->log = log;
>      dev->log_size = size;
> +
> +out:
> +    return 0;
>  }
> 
> 
> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
>  #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
>      /* To log more, must increase log size before table update. */
>      if (dev->log_size < log_size) {
> -        vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> +            abort();
> +        }
>      }
>      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
>      if (r < 0) {
> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
>      }
>      /* To log less, can only decrease log size after table update. */
>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> -        vhost_dev_log_resize(dev, log_size);
> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> +            abort();
> +        }
>      }
> 
>  out:


I think the suggested handling is
	error_report() and exit().
we also need to propagate errno. So how about passing in Error then?


> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
> int enable)
>          }
>          vhost_log_put(dev, false);
>      } else {
> -        vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> +        r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> +        if (r < 0) {
> +            return r;
> +        }
> +
>          r = vhost_dev_set_log(dev, true);
>          if (r < 0) {
>              return r;
> 
> 
> > 
> > .
> > 
> 
> -- 
> ---
> Regards,
> Longpeng(Mike)

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Longpeng (Mike)]

在 2020/3/10 16:23, Michael S. Tsirkin 写道:
> On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>>
>>
>> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
>>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
>>>> From: Longpeng <longpeng2@huawei.com>
>>>>
>>>> vhost_log_alloc() may fails and returned pointer of log is null.
>>>> However there're two places derefernce the return pointer without
>>>> check.
>>>>
[...]

>>>>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>>>>  {
>>>> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>>>> -    uint64_t log_base = (uintptr_t)log->log;
>>>> +    struct vhost_log *log;
>>>> +    uint64_t log_base;
>>>>      int r;
>>>>  
>>>> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>>>> +    if (!log) {
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> I'm not sure silently failing like this is safe. Callers assume
>>> log can be resized. What can be done? I suspect not much
>>> beside exiting ...
>>> Speaking of which, lots of other failures in log resizing
>>> path seem to be silently ignored.
>>> I guess we should propagate them, and fix callers to check
>>> the return code?
>>>
>> How about to let the callers treat the failure of log_resize as a fatal error ?
>>
[...]

>>
>> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
>>  #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
>>      /* To log more, must increase log size before table update. */
>>      if (dev->log_size < log_size) {
>> -        vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
>> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> +            abort();
>> +        }
>>      }
>>      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
>>      if (r < 0) {
>> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
>>      }
>>      /* To log less, can only decrease log size after table update. */
>>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
>> -        vhost_dev_log_resize(dev, log_size);
>> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> +            abort();
>> +        }
>>      }
>>
>>  out:
> 
> 
> I think the suggested handling is
> 	error_report() and exit().
> we also need to propagate errno. So how about passing in Error then?
> 
vhost_dev_log_resize
  vhost_log_get
    vhost_log_alloc
      error_report_err (fail path, errno is in the errp)
  VHOST_OPS_DEBUG (if ->vhost_set_log_base fail)
      error_report (errno)

Um, it seems log_resize will report error with errno internal, do we need
error_report once more ?

> 
>> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
>> int enable)
>>          }
>>          vhost_log_put(dev, false);
>>      } else {
>> -        vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> +        r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> +        if (r < 0) {
>> +            return r;
>> +        }
>> +
>>          r = vhost_dev_set_log(dev, true);
>>          if (r < 0) {
>>              return r;
>>
>>
>>>
>>> .
>>>
>>
>> -- 
>> ---
>> Regards,
>> Longpeng(Mike)
> 
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

Re: [PATCH RESEND 2/3] vhost: fix a null pointer reference of vhost_log

[Michael S. Tsirkin]

On Tue, Mar 10, 2020 at 08:02:30PM +0800, Longpeng (Mike) wrote:
> 在 2020/3/10 16:23, Michael S. Tsirkin 写道:
> > On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
> >>
> >>
> >> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> >>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> >>>> From: Longpeng <longpeng2@huawei.com>
> >>>>
> >>>> vhost_log_alloc() may fails and returned pointer of log is null.
> >>>> However there're two places derefernce the return pointer without
> >>>> check.
> >>>>
> [...]
> 
> >>>>  static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> >>>>  {
> >>>> -    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >>>> -    uint64_t log_base = (uintptr_t)log->log;
> >>>> +    struct vhost_log *log;
> >>>> +    uint64_t log_base;
> >>>>      int r;
> >>>>  
> >>>> +    log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >>>> +    if (!log) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>
> >>> I'm not sure silently failing like this is safe. Callers assume
> >>> log can be resized. What can be done? I suspect not much
> >>> beside exiting ...
> >>> Speaking of which, lots of other failures in log resizing
> >>> path seem to be silently ignored.
> >>> I guess we should propagate them, and fix callers to check
> >>> the return code?
> >>>
> >> How about to let the callers treat the failure of log_resize as a fatal error ?
> >>
> [...]
> 
> >>
> >> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
> >>  #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
> >>      /* To log more, must increase log size before table update. */
> >>      if (dev->log_size < log_size) {
> >> -        vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> >> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> >> +            abort();
> >> +        }
> >>      }
> >>      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> >>      if (r < 0) {
> >> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
> >>      }
> >>      /* To log less, can only decrease log size after table update. */
> >>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> >> -        vhost_dev_log_resize(dev, log_size);
> >> +        if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> >> +            abort();
> >> +        }
> >>      }
> >>
> >>  out:
> > 
> > 
> > I think the suggested handling is
> > 	error_report() and exit().
> > we also need to propagate errno. So how about passing in Error then?
> > 
> vhost_dev_log_resize
>   vhost_log_get
>     vhost_log_alloc
>       error_report_err (fail path, errno is in the errp)
>   VHOST_OPS_DEBUG (if ->vhost_set_log_base fail)
>       error_report (errno)
> 
> Um, it seems log_resize will report error with errno internal, do we need
> error_report once more ?

Well we need to convert this over to something other than
VHOST_OPS_DEBUG, that will go away at some point.

> > 
> >> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
> >> int enable)
> >>          }
> >>          vhost_log_put(dev, false);
> >>      } else {
> >> -        vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> >> +        r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> >> +        if (r < 0) {
> >> +            return r;
> >> +        }
> >> +
> >>          r = vhost_dev_set_log(dev, true);
> >>          if (r < 0) {
> >>              return r;
> >>
> >>
> >>>
> >>> .
> >>>
> >>
> >> -- 
> >> ---
> >> Regards,
> >> Longpeng(Mike)
> > 
> > 
> > .
> > 
> 
> 
> -- 
> Regards,
> Longpeng(Mike)

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.)"
<longpeng2@huawei.com> wrote:

> On 2020/2/25 0:04, Alex Williamson wrote:
> > On Mon, 24 Feb 2020 14:42:17 +0800
> > "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> >   
> >> From: Longpeng <longpeng2@huawei.com>
> >>
> >> 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 <longpeng2@huawei.com>
> >> ---
> >>  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 <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:

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 <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.

Thanks
Laszlo

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

[Alex Williamson]

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,

Alex

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

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Mon, 24 Feb 2020 14:42:17 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
>
>> From: Longpeng <longpeng2@huawei.com>
>> 
>> 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.

What "VM abort" exactly?

>> 
>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> ---
>>  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)
    {
        VFIOPCIDevice *vdev = opaque;
        union {
            uint8_t byte;
            uint16_t word;
            uint32_t dword;
            uint64_t qword;
        } val;
>>      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,

I'm blind: why does !vdev->rom imply !vdev->rom_size?

Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read
uninitialized data from @val:

        switch (size) {
        case 1:
            data = val.byte;
            break;
        case 2:
            data = le16_to_cpu(val.word);
            break;
        case 4:
            data = le32_to_cpu(val.dword);
            break;
        default:
            hw_error("vfio: unsupported read size, %d bytes\n", size);
            break;
        }

        trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data);

        return data;
    }

Why is that okay?

Why do we initialize @data?

How can we get to the default case?  If we can get there, is hw_error()
really the right thing to do?  It almost never is...  If getting there
is the guest's fault, we need to tell it off the same way physical
hardware does.  If we should not ever get there (i.e. it's a QEMU bug),
then a plain abort() would be clearer.

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

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 11 Mar 2020 00:14:31 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
[...]
>> 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,

We rely on GNU C extensions all over theplace.  Making the code uglier
to avoid relying on this one here makes no sense to me.

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

[Laszlo Ersek]

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! :)

Thanks
Laszlo

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

[Laszlo Ersek]

On 03/11/20 08:08, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
>> On Wed, 11 Mar 2020 00:14:31 +0100
>> Laszlo Ersek <lersek@redhat.com> wrote:
> [...]
>>> 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,
> 
> We rely on GNU C extensions all over theplace.  Making the code uglier
> to avoid relying on this one here makes no sense to me.
> 

I agree, in fact. If GNU-isms are liberally used & tolerated in the QEMU
source, then there's no reason to diverge from that here. I steer clear
of GNU-isms as much as I can, regardless of codebase, but I *did* forget
that QEMU permits GNU-isms -- so there's no need for my pedantry here.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

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

[Markus Armbruster]

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'.

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

[Laszlo Ersek]

On 03/11/20 12:54, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:

>> 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'.
> 

Thanks! It seems like "-Wpedantic" and "-pedantic" are synonymous. And
the latter used to be part of my standard set of flags, while I worked
on hosted C programs where I could influence the build flags ;)

Cheers,
Laszlo

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

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 11 Mar 2020 08:04:28 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> 
>> > On Mon, 24 Feb 2020 14:42:17 +0800
>> > "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
>> >  
>> >> From: Longpeng <longpeng2@huawei.com>
>> >> 
>> >> 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.  
>> 
>> What "VM abort" exactly?
>
> There is none because memcpy() does something sane when size is zero,
> but to be ISO whatever spec compliant we shouldn't rely on that.
>
>> >> 
>> >> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> >> ---
>> >>  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)  
>>     {
>>         VFIOPCIDevice *vdev = opaque;
>>         union {
>>             uint8_t byte;
>>             uint16_t word;
>>             uint32_t dword;
>>             uint64_t qword;
>>         } val;
>> >>      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,  
>> 
>> I'm blind: why does !vdev->rom imply !vdev->rom_size?
>
> See vfio_pci_load_rom(), rom_size and rom are set and allocated
> together.

What if vfio_pci_load_rom() isn't called, or returns before it sets
these guys?

>> Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read
>> uninitialized data from @val:
>
> This is fixed in my patch
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02778.html

Yes.

>> 
>>         switch (size) {
>>         case 1:
>>             data = val.byte;
>>             break;
>>         case 2:
>>             data = le16_to_cpu(val.word);
>>             break;
>>         case 4:
>>             data = le32_to_cpu(val.dword);
>>             break;
>>         default:
>>             hw_error("vfio: unsupported read size, %d bytes\n", size);
>>             break;
>>         }
>> 
>>         trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data);
>> 
>>         return data;
>>     }
>> 
>> Why is that okay?
>> 
>> Why do we initialize @data?
>
> Bug.  The switch was only added later (75bd0c7253f3) and we failed to
> catch it.  Prior to that we were initializing val and the memcpy() only
> overwrote it as necessary.  In any case, getting back garbage for the
> rom when there isn't one generally works ok since the chances of
> generating a proper rom signature are infinitesimal.  Clearly not what
> was intended though.
>
>> How can we get to the default case?  If we can get there, is hw_error()
>> really the right thing to do?  It almost never is...  If getting there
>> is the guest's fault, we need to tell it off the same way physical
>> hardware does.  If we should not ever get there (i.e. it's a QEMU bug),
>> then a plain abort() would be clearer.
>
> AFAIK this is relatively standard, if not somewhat paranoid, handling
> for a MemoryRegion ops callback.  The MemoryRegionOps code only allows
> certain size accesses, so it would effectively be an internal error to
> hit the default case, which seems to be not an uncommon use case of
> hw_error.  Thanks,

Using hw_error() for such programming errors is not helpful.  Everything
it adds to abort() is useless or misleading.

In fact, most uses of hw_error() are not helpful.

But you're going with the flow here.  I accept that.

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

[Alex Williamson]

On Thu, 12 Mar 2020 06:50:30 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Wed, 11 Mar 2020 08:04:28 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >  
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >>   
> >> > On Mon, 24 Feb 2020 14:42:17 +0800
> >> > "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> >> >    
> >> >> From: Longpeng <longpeng2@huawei.com>
> >> >> 
> >> >> 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.    
> >> 
> >> What "VM abort" exactly?  
> >
> > There is none because memcpy() does something sane when size is zero,
> > but to be ISO whatever spec compliant we shouldn't rely on that.
> >  
> >> >> 
> >> >> Signed-off-by: Longpeng <longpeng2@huawei.com>
> >> >> ---
> >> >>  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)    
> >>     {
> >>         VFIOPCIDevice *vdev = opaque;
> >>         union {
> >>             uint8_t byte;
> >>             uint16_t word;
> >>             uint32_t dword;
> >>             uint64_t qword;
> >>         } val;  
> >> >>      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,    
> >> 
> >> I'm blind: why does !vdev->rom imply !vdev->rom_size?  
> >
> > See vfio_pci_load_rom(), rom_size and rom are set and allocated
> > together.  
> 
> What if vfio_pci_load_rom() isn't called, or returns before it sets
> these guys?

vfio_pci_load_rom() is called from this read function if they're not
set and we haven't triggered a read failure.  They're zero allocated.
The intention is that any vfio_pci_load_rom() failure will leave these
unset and trigger a zero sized memcpy, or now skip the mempcy as in the
patch below.  Thanks,

Alex

> >> Moreover, when MIN(size, vdev->rom_size - addr) < size, we seem to read
> >> uninitialized data from @val:  
> >
> > This is fixed in my patch
> > https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg02778.html  
> 
> Yes.
> 
> >> 
> >>         switch (size) {
> >>         case 1:
> >>             data = val.byte;
> >>             break;
> >>         case 2:
> >>             data = le16_to_cpu(val.word);
> >>             break;
> >>         case 4:
> >>             data = le32_to_cpu(val.dword);
> >>             break;
> >>         default:
> >>             hw_error("vfio: unsupported read size, %d bytes\n", size);
> >>             break;
> >>         }
> >> 
> >>         trace_vfio_rom_read(vdev->vbasedev.name, addr, size, data);
> >> 
> >>         return data;
> >>     }
> >> 
> >> Why is that okay?
> >> 
> >> Why do we initialize @data?  
> >
> > Bug.  The switch was only added later (75bd0c7253f3) and we failed to
> > catch it.  Prior to that we were initializing val and the memcpy() only
> > overwrote it as necessary.  In any case, getting back garbage for the
> > rom when there isn't one generally works ok since the chances of
> > generating a proper rom signature are infinitesimal.  Clearly not what
> > was intended though.
> >  
> >> How can we get to the default case?  If we can get there, is hw_error()
> >> really the right thing to do?  It almost never is...  If getting there
> >> is the guest's fault, we need to tell it off the same way physical
> >> hardware does.  If we should not ever get there (i.e. it's a QEMU bug),
> >> then a plain abort() would be clearer.  
> >
> > AFAIK this is relatively standard, if not somewhat paranoid, handling
> > for a MemoryRegion ops callback.  The MemoryRegionOps code only allows
> > certain size accesses, so it would effectively be an internal error to
> > hit the default case, which seems to be not an uncommon use case of
> > hw_error.  Thanks,  
> 
> Using hw_error() for such programming errors is not helpful.  Everything
> it adds to abort() is useless or misleading.
> 
> In fact, most uses of hw_error() are not helpful.
> 
> But you're going with the flow here.  I accept that.