[block-xen-blkback] question about pontential null pointer dereference

[block-xen-blkback] question about pontential null pointer dereference

[Gustavo A. R. Silva]

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?

I'm trying to figure out if this is a false positive or something that  
actually needs to be fixed.

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

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

[Juergen Gross]

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

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

[Gustavo A. R. Silva]

Hi Juergen,

Quoting Juergen Gross <jgross@suse.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

[PATCH] block: xen-blkback: add null check to avoid null pointer dereference

[Gustavo A. R. Silva]

Add null check before calling xen_blkif_put() to avoid potential
null pointer dereference.

Addresses-Coverity-ID: 1350942
Cc: Juergen Gross <jgross@suse.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/block/xen-blkback/xenbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 8fe61b5..1f3dfaa 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -504,11 +504,13 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
 
 	dev_set_drvdata(&dev->dev, NULL);
 
-	if (be->blkif)
+	if (be->blkif) {
 		xen_blkif_disconnect(be->blkif);
 
-	/* Put the reference we set in xen_blkif_alloc(). */
-	xen_blkif_put(be->blkif);
+		/* Put the reference we set in xen_blkif_alloc(). */
+		xen_blkif_put(be->blkif);
+	}
+
 	kfree(be->mode);
 	kfree(be);
 	return 0;
-- 
2.5.0

Re: [PATCH] block: xen-blkback: add null check to avoid null pointer dereference

[Konrad Rzeszutek Wilk]

On Thu, May 11, 2017 at 10:27:35AM -0500, Gustavo A. R. Silva wrote:
> Add null check before calling xen_blkif_put() to avoid potential
> null pointer dereference.
> 

Applied to 'stable/for-jens-4.12' and will push soon to Jens.

> Addresses-Coverity-ID: 1350942
> Cc: Juergen Gross <jgross@suse.com>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/block/xen-blkback/xenbus.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 8fe61b5..1f3dfaa 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -504,11 +504,13 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>  
>  	dev_set_drvdata(&dev->dev, NULL);
>  
> -	if (be->blkif)
> +	if (be->blkif) {
>  		xen_blkif_disconnect(be->blkif);
>  
> -	/* Put the reference we set in xen_blkif_alloc(). */
> -	xen_blkif_put(be->blkif);
> +		/* Put the reference we set in xen_blkif_alloc(). */
> +		xen_blkif_put(be->blkif);
> +	}
> +
>  	kfree(be->mode);
>  	kfree(be);
>  	return 0;
> -- 
> 2.5.0
>