Re: [Xen-devel] [block-xen-blkback] question about pontential null pointer dereference

[Gustavo A. R. Silva]

Hi Juergen,

Quoting Juergen Gross <jgross@xxxxxxxx>:

> On 10/05/17 18:49, Gustavo A. R. Silva wrote:
> > Hello everybody,
> >
> > While looking into Coverity ID 1350942 I ran into the following piece of
> > code at drivers/block/xen-blkback/xenbus.c:490:
> >
> > 490static int xen_blkbk_remove(struct xenbus_device *dev)
> > 491{
> > 492        struct backend_info *be = dev_get_drvdata(&dev->dev);
> > 493
> > 494        pr_debug("%s %p %d\n", __func__, dev, dev->otherend_id);
> > 495
> > 496        if (be->major || be->minor)
> > 497                xenvbd_sysfs_delif(dev);
> > 498
> > 499        if (be->backend_watch.node) {
> > 500                unregister_xenbus_watch(&be->backend_watch);
> > 501                kfree(be->backend_watch.node);
> > 502                be->backend_watch.node = NULL;
> > 503        }
> > 504
> > 505        dev_set_drvdata(&dev->dev, NULL);
> > 506
> > 507        if (be->blkif)
> > 508                xen_blkif_disconnect(be->blkif);
> > 509
> > 510        /* Put the reference we set in xen_blkif_alloc(). */
> > 511        xen_blkif_put(be->blkif);
> > 512        kfree(be->mode);
> > 513        kfree(be);
> > 514        return 0;
> > 515}
> >
> > The issue here is that line 507 implies that be->blkif might be NULL. If
> > this is the case, there is a NULL pointer dereference when executing
> > line 511 once macro xen_blkif_put() dereference be->blkif
> >
> > Is there any chance for be->blkif to be NULL at line 511?
>
> Yes. xen_blkbk_probe() will call xen_blkbk_remove() with be->blkif being
> NULL in the failure path.
>
> The call to xen_blkif_put() should be guarded by the "if (be->blkif)" of
> line 507, too.

Thanks for clarifying. I'll send a patch to fix this shortly.

--
Gustavo A. R. Silva