* [block-xen-blkback] question about pontential null pointer dereference
@ 2017-05-10 16:49 Gustavo A. R. Silva
2017-05-11 8:53 ` Juergen Gross
2017-05-11 8:53 ` [Xen-devel] " Juergen Gross
0 siblings, 2 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-10 16:49 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Roger Pau Monné; +Cc: xen-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [block-xen-blkback] question about pontential null pointer dereference
2017-05-10 16:49 [block-xen-blkback] question about pontential null pointer dereference Gustavo A. R. Silva
2017-05-11 8:53 ` Juergen Gross
@ 2017-05-11 8:53 ` Juergen Gross
2017-05-11 15:05 ` Gustavo A. R. Silva
1 sibling, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2017-05-11 8:53 UTC (permalink / raw)
To: Gustavo A. R. Silva, Konrad Rzeszutek Wilk, Roger Pau Monné
Cc: xen-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [block-xen-blkback] question about pontential null pointer dereference
2017-05-10 16:49 [block-xen-blkback] question about pontential null pointer dereference Gustavo A. R. Silva
@ 2017-05-11 8:53 ` Juergen Gross
2017-05-11 8:53 ` [Xen-devel] " Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-05-11 8:53 UTC (permalink / raw)
To: Gustavo A. R. Silva, Konrad Rzeszutek Wilk, Roger Pau Monné
Cc: xen-devel, linux-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [block-xen-blkback] question about pontential null pointer dereference
2017-05-11 8:53 ` [Xen-devel] " Juergen Gross
@ 2017-05-11 15:05 ` Gustavo A. R. Silva
0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-11 15:05 UTC (permalink / raw)
To: Juergen Gross
Cc: Konrad Rzeszutek Wilk, Roger Pau Monné, xen-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [block-xen-blkback] question about pontential null pointer dereference
@ 2017-05-11 15:05 ` Gustavo A. R. Silva
0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-11 15:05 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, Roger Pau Monné, linux-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] block: xen-blkback: add null check to avoid null pointer dereference
2017-05-11 15:05 ` Gustavo A. R. Silva
@ 2017-05-11 15:27 ` Gustavo A. R. Silva
-1 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-11 15:27 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Roger Pau Monné, Juergen Gross
Cc: xen-devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] block: xen-blkback: add null check to avoid null pointer dereference
@ 2017-05-11 15:27 ` Gustavo A. R. Silva
0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-11 15:27 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Roger Pau Monné, Juergen Gross
Cc: xen-devel, Gustavo A. R. Silva, linux-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] block: xen-blkback: add null check to avoid null pointer dereference
2017-05-11 15:27 ` Gustavo A. R. Silva
(?)
@ 2017-05-15 21:18 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-05-15 21:18 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Roger Pau Monné, Juergen Gross, xen-devel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: xen-blkback: add null check to avoid null pointer dereference
2017-05-11 15:27 ` Gustavo A. R. Silva
(?)
(?)
@ 2017-05-15 21:18 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-05-15 21:18 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Juergen Gross, xen-devel, linux-kernel, Roger Pau Monné
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [block-xen-blkback] question about pontential null pointer dereference
@ 2017-05-10 16:49 Gustavo A. R. Silva
0 siblings, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-10 16:49 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Roger Pau Monné; +Cc: xen-devel, linux-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-15 21:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 16:49 [block-xen-blkback] question about pontential null pointer dereference Gustavo A. R. Silva
2017-05-11 8:53 ` Juergen Gross
2017-05-11 8:53 ` [Xen-devel] " Juergen Gross
2017-05-11 15:05 ` Gustavo A. R. Silva
2017-05-11 15:05 ` Gustavo A. R. Silva
2017-05-11 15:27 ` [PATCH] block: xen-blkback: add null check to avoid " Gustavo A. R. Silva
2017-05-11 15:27 ` Gustavo A. R. Silva
2017-05-15 21:18 ` Konrad Rzeszutek Wilk
2017-05-15 21:18 ` Konrad Rzeszutek Wilk
2017-05-10 16:49 [block-xen-blkback] question about pontential " Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.