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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170511100544.Horde.88c0RhMPEXYq42BauaUXfHV@gator4166.hostgator.com>
Date:   Thu, 11 May 2017 10:05:44 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Juergen Gross <jgross@...e.com>
Cc:     Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Roger Pau Monné <roger.pau@...rix.com>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Subject: Re: [Xen-devel] [block-xen-blkback] question about pontential null
 pointer dereference

Hi Juergen,

Quoting Juergen Gross <jgross@...e.com>:

> 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