* [block-xen-blkback] question about pontential null pointer dereference
@ 2017-05-10 16:49 Gustavo A. R. Silva
2017-05-11 8:53 ` [Xen-devel] " Juergen Gross
0 siblings, 1 reply; 5+ 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] 5+ 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 15:05 ` Gustavo A. R. Silva
0 siblings, 1 reply; 5+ 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] 5+ 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
2017-05-11 15:27 ` [PATCH] block: xen-blkback: add null check to avoid " Gustavo A. R. Silva
0 siblings, 1 reply; 5+ 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] 5+ 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
2017-05-15 21:18 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH] block: xen-blkback: add null check to avoid null pointer dereference
2017-05-11 15:27 ` [PATCH] block: xen-blkback: add null check to avoid " Gustavo A. R. Silva
@ 2017-05-15 21:18 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2017-05-15 21:19 UTC | newest]
Thread overview: 5+ 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 ` [Xen-devel] " Juergen Gross
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-15 21:18 ` Konrad Rzeszutek Wilk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).