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

Olaf Hering

author

2012-12-03 19:32 UTC #0 lore raw

PATCH 1/1 xen/blkback: do not leak mode property

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 <[email protected]>

---

!! Not compile tested !!

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

drivers/block/xen-blkback/xenbus.c +1 -0

index f58434c..a6585a4 100644
@@ -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;

Signature · 1 line

1.8.0.1

Jan Beulich

2012-12-04 10:30 UTC #1 lore raw

Quoted 19 lines

Quoted 1 line
Quoted 1 line · 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 <[email protected]>
---
!! Not compile tested !!
 drivers/block/xen-blkback/xenbus.c | 1 +
 1 file changed, 1 insertion(+)
drivers/block/xen-blkback/xenbus.c +0 -0
index f58434c..a6585a4 100644

drivers/block/xen-blkback/xenbus.c static int xen_blkbk_remove(struct xenbus_device *dev) · line 369

Quoted 4 lines

drivers/block/xen-blkback/xenbus.c +1 -0

@@ -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

Quoted 5 lines

drivers/block/xen-blkback/xenbus.c +0 -0
 	kfree(be);
 	dev_set_drvdata(&dev->dev, NULL);
 	return 0;
Signature · 1 line
1.8.0.1

Jan Beulich

2012-12-04 10:30 UTC #2 lore raw

In reply to Olaf Hering

Quoted 19 lines

Quoted 1 line
Quoted 1 line · 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 <[email protected]>
---
!! Not compile tested !!
 drivers/block/xen-blkback/xenbus.c | 1 +
 1 file changed, 1 insertion(+)
drivers/block/xen-blkback/xenbus.c +0 -0
index f58434c..a6585a4 100644

drivers/block/xen-blkback/xenbus.c static int xen_blkbk_remove(struct xenbus_device *dev) · line 369

Quoted 4 lines

drivers/block/xen-blkback/xenbus.c +1 -0

@@ -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

Quoted 5 lines

drivers/block/xen-blkback/xenbus.c +0 -0
 	kfree(be);
 	dev_set_drvdata(&dev->dev, NULL);
 	return 0;
Signature · 1 line
1.8.0.1

Olaf Hering

author

2012-12-04 18:21 UTC #3 lore raw

Quoted 8 lines · Jan Beulich

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

Olaf Hering

author

2012-12-04 18:21 UTC #4 lore raw

In reply to Jan Beulich

Quoted 8 lines · Jan Beulich

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

Jan Beulich

2012-12-05 08:00 UTC #5 lore raw

Quoted 12 lines

Quoted 1 line
Quoted 1 line · Olaf Hering
Quoted 8 lines · Jan Beulich
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).

Quoted 7 lines

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

Jan Beulich

2012-12-05 08:01 UTC #6 lore raw

In reply to Olaf Hering

Quoted 12 lines

Quoted 1 line
Quoted 1 line · Olaf Hering
Quoted 8 lines · Jan Beulich
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).

Quoted 7 lines

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