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 <address@hidden>
> >>
> >> 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 <address@hidden>
> >> ---
> >>  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)