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 <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.
> >>>>
> [...]
> 
> >>>>  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)