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 <longpe...@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 <longpe...@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)