[PATCH] xen/blkback: do not leak mode property

[PATCH] xen/blkback: do not leak mode property

[Olaf Hering]

be->mode is obtained from xenbus_read, which does a kmalloc for the
message body. The short string is never released, so do it on blkbk
remove.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---

!! Not compile tested !!

 drivers/block/xen-blkback/xenbus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index f58434c..a6585a4 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
 		be->blkif = NULL;
 	}
 
+	kfree(be->mode);
 	kfree(be);
 	dev_set_drvdata(&dev->dev, NULL);
 	return 0;
-- 
1.8.0.1

Re: [PATCH] xen/blkback: do not leak mode property

[Jan Beulich]

>>> On 03.12.12 at 20:32, Olaf Hering <olaf@aepfle.de> wrote:
> be->mode is obtained from xenbus_read, which does a kmalloc for the
> message body. The short string is never released, so do it on blkbk
> remove.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> 
> !! Not compile tested !!
> 
>  drivers/block/xen-blkback/xenbus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index f58434c..a6585a4 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev)
>  		be->blkif = NULL;
>  	}
>  
> +	kfree(be->mode);

This looks necessary but insufficient - there's nothing really
preventing backend_changed() from being called more than once
for a given device (is simply the handler of xenbus watch). Hence
I think either that function needs to be guarded against multiple
execution (e.g. by removing the watch from that function itself,
if that's permitted by xenbus), or to properly deal with the
effects this has (including but probably not limited to the leaking
of be->mode).

Jan

>  	kfree(be);
>  	dev_set_drvdata(&dev->dev, NULL);
>  	return 0;
> -- 
> 1.8.0.1

Re: [PATCH] xen/blkback: do not leak mode property

[Olaf Hering]

On Tue, Dec 04, Jan Beulich wrote:

> This looks necessary but insufficient - there's nothing really
> preventing backend_changed() from being called more than once
> for a given device (is simply the handler of xenbus watch). Hence
> I think either that function needs to be guarded against multiple
> execution (e.g. by removing the watch from that function itself,
> if that's permitted by xenbus), or to properly deal with the
> effects this has (including but probably not limited to the leaking
> of be->mode).

If another watch does really trigger after the kfree(be) in
xen_blkbk_remove(), wouldnt backend_changed access stale memory?
So if that can really happen in practice, shouldnt the backend_watch be
a separate allocation instead being contained within backend_info?

Looking at unregister_xenbus_watch, it clears removes the watch from the
list, so that process_msg will not see it anymore.

Olaf

Re: [PATCH] xen/blkback: do not leak mode property

[Jan Beulich]

>>> On 04.12.12 at 19:21, Olaf Hering <olaf@aepfle.de> wrote:
> On Tue, Dec 04, Jan Beulich wrote:
> 
>> This looks necessary but insufficient - there's nothing really
>> preventing backend_changed() from being called more than once
>> for a given device (is simply the handler of xenbus watch). Hence
>> I think either that function needs to be guarded against multiple
>> execution (e.g. by removing the watch from that function itself,
>> if that's permitted by xenbus), or to properly deal with the
>> effects this has (including but probably not limited to the leaking
>> of be->mode).
> 
> If another watch does really trigger after the kfree(be) in
> xen_blkbk_remove(), wouldnt backend_changed access stale memory?
> So if that can really happen in practice, shouldnt the backend_watch be
> a separate allocation instead being contained within backend_info?
> 
> Looking at unregister_xenbus_watch, it clears removes the watch from the
> list, so that process_msg will not see it anymore.

That's not the scenario I was talking about: I'm concerned about
multiple calls to backend_changed() to similarly leak "mode" (and
possibly cause other bad stuff to happen) while the device is still
alive - after all it overwrites "mode" without checking what's in
there.

Jan