question about pontential null pointer dereference

block-xen-blkback Applied Gustavo A. R. Silva 3 participants 7 messages May 10, 2017 – May 15, 2017

Subsystems (2)

Block layer Xen block

lore

Share & tools

lore

Gustavo A. R. Silva

author

2017-05-10 16:49 UTC #0 lore raw

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

Juergen Gross

2017-05-11 08:53 UTC #1 lore raw

Quoted 31 lines · 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;

Quoted 7 lines

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

Gustavo A. R. Silva

author

2017-05-11 15:05 UTC #2 lore raw

Hi Juergen,

Quoting Juergen Gross <[email protected]>:

Quoted 38 lines · Gustavo A. R. Silva

Quoted 37 lines
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

Quoted 8 lines

Quoted 1 line
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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Gustavo A. R. Silva

author

2017-05-11 15:27 UTC #3 lore raw

PATCH 1/1 block: xen-blkback: add null check to avoid null pointer dereference

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

Addresses-Coverity-ID: 1350942

Cc Juergen Gross <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/block/xen-blkback/xenbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/xenbus.c +5 -3

index 8fe61b5..1f3dfaa 100644
@@ -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;

Signature · 7 lines

2.5.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Konrad Rzeszutek Wilk

Applied

2017-05-15 21:19 UTC #4 lore raw

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

Quoted 3 lines · Gustavo A. R. Silva

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.

Quoted 31 lines

Addresses-Coverity-ID: 1350942

Cc Juergen Gross <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/block/xen-blkback/xenbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/xenbus.c +5 -3

index 8fe61b5..1f3dfaa 100644
@@ -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;

Signature · 1 line

2.5.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Konrad Rzeszutek Wilk

Applied

2017-05-15 21:19 UTC #5 lore raw

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

In reply to Gustavo A. R. Silva

Quoted 3 lines · Gustavo A. R. Silva

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.

Quoted 31 lines

Addresses-Coverity-ID: 1350942

Cc Juergen Gross <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/block/xen-blkback/xenbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/block/xen-blkback/xenbus.c +5 -3

index 8fe61b5..1f3dfaa 100644
@@ -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;

Signature · 1 line

2.5.0

Juergen Gross

2017-05-11 08:53 UTC #6 lore raw

In reply to Gustavo A. R. Silva

Quoted 31 lines · 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;

Quoted 7 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel