From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932294AbdEKIxh (ORCPT <rfc822;w@1wt.eu>);
        Thu, 11 May 2017 04:53:37 -0400
Received: from mx2.suse.de ([195.135.220.15]:54179 "EHLO mx1.suse.de"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S932079AbdEKIxg (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 11 May 2017 04:53:36 -0400
Subject: Re: [Xen-devel] [block-xen-blkback] question about pontential null
 pointer dereference
To: "Gustavo A. R. Silva" <garsilva@embeddedor.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <roger.pau@citrix.com>
References: <20170510114953.Horde.7C1VjUQI-YiZdSL2tKfFVHL@gator4166.hostgator.com>
Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org
From: Juergen Gross <jgross@suse.com>
Message-ID: <e4387fb6-9cf4-eca4-06ce-d29f300d0c7b@suse.com>
Date: Thu, 11 May 2017 10:53:33 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170510114953.Horde.7C1VjUQI-YiZdSL2tKfFVHL@gator4166.hostgator.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.


Juergen