diff mbox

Hardy SRU, xen unified block-device I/O interface back end can orphan devices, CVE-2010-3699

Message ID 4D4060AA.6070404@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Stefan Bader Jan. 26, 2011, 5:58 p.m. UTC 
On 01/26/2011 05:45 PM, Stefan Bader wrote:
> Scary, this sort of pulls in silently a few other changes to Xen, which makes it
> even harder.
> 
> On 01/26/2011 03:16 PM, Tim Gardner wrote:
>> The following changes since commit 11e175fe9030a484684a34f0b13040bf93ef2290:
>>   Dan Rosenberg (1):
>>         V4L/DVB: ivtvfb: prevent reading uninitialized stack memory, CVE-2010-4079
>>
>> are available in the git repository at:
>>
>>   git://kernel.ubuntu.com/rtg/ubuntu-hardy.git CVE-2010-3699
>>
>> Tim Gardner (1):
>>       UBUNTU: xen unified block-device I/O interface back end can orphan devices, CVE-2010-3699
>>
>>  .../xen/patchset/020-xen-CVE-2010-3699.patch       |  152 ++++++++++++++++++++
>>  1 files changed, 152 insertions(+), 0 deletions(-)
>>  create mode 100644 debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
>>
>> From a42d61170c261228e31c95b98e42ec80444638d8 Mon Sep 17 00:00:00 2001
>> From: Tim Gardner <tim.gardner@canonical.com>
>> Date: Tue, 25 Jan 2011 14:54:33 -0700
>> Subject: [PATCH] UBUNTU: xen unified block-device I/O interface back end can orphan devices, CVE-2010-3699
>>
>> Excerpted from http://rhn.redhat.com/errata/RHSA-2011-0004.html:
>>
>> "A flaw was found in the Xenbus code for the unified block-device I/O
>> interface back end. A privileged guest user could use this flaw to cause a
>> denial of service on the host system running the Xen hypervisor."
>>
>> Most of this patch was developed from http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/59f097ef181b.
>> A careful comparision of the patch and the actual contents of http://xenbits.xensource.com/linux-2.6.18-xen.hg
>> showed that part of the original patch introduced a regression as can be seen from the Mercurial log.
>> I've not implemented that piece of the patch.
>>
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>> ---
>>  .../xen/patchset/020-xen-CVE-2010-3699.patch       |  152 ++++++++++++++++++++
>>  1 files changed, 152 insertions(+), 0 deletions(-)
>>  create mode 100644 debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
>>
>> diff --git a/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch b/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
>> new file mode 100644
>> index 0000000..57dea44
>> --- /dev/null
>> +++ b/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
>> @@ -0,0 +1,152 @@
>> +diff --git a/drivers/xen/blkback/xenbus.c b/drivers/xen/blkback/xenbus.c
>> +index f9feb43..1611d7a 100644
>> +--- a/drivers/xen/blkback/xenbus.c
>> ++++ b/drivers/xen/blkback/xenbus.c
>> +@@ -362,6 +362,11 @@ static void frontend_changed(struct xenbus_device *dev,
>> + 		if (dev->state == XenbusStateConnected)
>> + 			break;
>> + 
>> ++		/* Enforce precondition before potential leak point.
>> ++		 * blkif_disconnect() is idempotent.
>> ++		 */
>> ++		blkif_disconnect(be->blkif);
>> ++
>> + 		err = connect_ring(be);
>> + 		if (err)
>> + 			break;
>> +@@ -379,6 +384,7 @@ static void frontend_changed(struct xenbus_device *dev,
>> + 			break;
>> + 		/* fall through if not online */
>> + 	case XenbusStateUnknown:
>> ++		/* implies blkif_disconnect() via blkback_remove() */
>> + 		device_unregister(&dev->dev);
>> + 		break;
>> + 
>> +diff --git a/drivers/xen/blktap/xenbus.c b/drivers/xen/blktap/xenbus.c
>> +index f9e3159..d5db1b5 100644
>> +--- a/drivers/xen/blktap/xenbus.c
>> ++++ b/drivers/xen/blktap/xenbus.c
>> +@@ -326,6 +326,18 @@ static void tap_backend_changed(struct xenbus_watch *watch,
>> + 	tap_update_blkif_status(be->blkif);
>> + }
>> + 
>> ++
>> ++static void tap_blkif_disconnect(blkif_t *blkif)
>> ++{
>> ++	if (blkif->xenblkd) {
>> ++		kthread_stop(blkif->xenblkd);
>> ++		blkif->xenblkd = NULL;
>> ++	}
>> ++
>> ++	/* idempotent */
> 
>> ++	tap_blkif_free(blkif);
> 
> Maybe this is giving grief. In our version tap_blkif_free() is also calling
> kmem_cache_free, while the version in the repo has this factored out into a
> separate function which is only called on blktap_remove()
> 
>> ++}
>> ++
>> + /**
>> +  * Callback received when the frontend's state changes.
>> +  */
>> +@@ -354,6 +366,11 @@ static void tap_frontend_changed(struct xenbus_device *dev,
>> + 		if (dev->state == XenbusStateConnected)
>> + 			break;
>> + 
>> ++		/* Enforce precondition before potential leak point.
>> ++		 * tap_blkif_disconnect() is idempotent.
>> ++		 */
>> ++		tap_blkif_disconnect(be->blkif);
>> ++
>> + 		err = connect_ring(be);
>> + 		if (err)
>> + 			break;
>> +@@ -361,10 +378,7 @@ static void tap_frontend_changed(struct xenbus_device *dev,
>> + 		break;
>> + 
>> + 	case XenbusStateClosing:
>> +-		if (be->blkif->xenblkd) {
>> +-			kthread_stop(be->blkif->xenblkd);
>> +-			be->blkif->xenblkd = NULL;
>> +-		}
>> ++		tap_blkif_disconnect(be->blkif);
>> + 		xenbus_switch_state(dev, XenbusStateClosing);
>> + 		break;
>> + 
>> +@@ -374,6 +388,9 @@ static void tap_frontend_changed(struct xenbus_device *dev,
>> + 			break;
>> + 		/* fall through if not online */
>> + 	case XenbusStateUnknown:
>> ++		/* Implies the effects of blkif_disconnect() via
>> ++		 * blktap_remove().
>> ++		 */
>> + 		device_unregister(&dev->dev);
>> + 		break;
>> + 
>> +diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
>> +index b55005c..fb84b4b 100644
>> +--- a/drivers/xen/netback/xenbus.c
>> ++++ b/drivers/xen/netback/xenbus.c
>> +@@ -32,6 +32,7 @@
>> + static int connect_rings(struct backend_info *);
>> + static void connect(struct backend_info *);
>> + static void backend_create_netif(struct backend_info *be);
>> ++static void netback_disconnect(struct device *);
>> + 
>> + static int netback_remove(struct xenbus_device *dev)
>> + {
>> +@@ -39,15 +40,22 @@ static int netback_remove(struct xenbus_device *dev)
>> + 
>> + 	netback_remove_accelerators(be, dev);
>> + 
>> +-	if (be->netif) {
>> +-		netif_disconnect(be->netif);
>> +-		be->netif = NULL;
>> +-	}
>> ++	netback_disconnect(&dev->dev);
>> + 	kfree(be);
>> + 	dev->dev.driver_data = NULL;
>> + 	return 0;
>> + }
>> + 
>> ++static void netback_disconnect(struct device *xbdev_dev)
>> ++{
>> ++        struct backend_info *be = xbdev_dev->driver_data;
>> ++ 
>> ++        if (be->netif) {
>> ++                kobject_uevent(&xbdev_dev->kobj, KOBJ_OFFLINE);
>> ++                netif_disconnect(be->netif);
>> ++                be->netif = NULL;
>> ++        }
>> ++}
>> + 
>> + /**
>> +  * Entry point to this code when a new device is created.  Allocate the basic
>> +@@ -225,16 +233,17 @@ static void frontend_changed(struct xenbus_device *dev,
>> + 		break;
>> + 
>> + 	case XenbusStateConnected:
>> ++		if (dev->state == XenbusStateConnected)
>> ++			break;
>> ++
>> ++		/* backend_create_netif() is idempotent */
>> + 		backend_create_netif(be);
>> + 		if (be->netif)
>> + 			connect(be);
>> + 		break;
>> + 
>> + 	case XenbusStateClosing:
>> +-		if (be->netif) {
>> +-			netif_disconnect(be->netif);
>> +-			be->netif = NULL;
>> +-		}
>> ++		netback_disconnect(&dev->dev);
>> + 		xenbus_switch_state(dev, XenbusStateClosing);
>> + 		break;
>> + 
>> +@@ -244,8 +253,7 @@ static void frontend_changed(struct xenbus_device *dev,
>> + 			break;
>> + 		/* fall through if not online */
>> + 	case XenbusStateUnknown:
>> +-		if (be->netif != NULL)
>> +-			kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
>> ++		/* implies netback_disconnect() via netback_remove() */
>> + 		device_unregister(&dev->dev);
>> + 		break;
>> + 
> 
> 
I still have the problem of running "xm create" but the same issue happens when
I use the kernel from current proposed. So that needs more investigation.
This is the patch I have come up so far.
diff mbox

Patch

From 25a05d703385a09feb95cce60c63b7078b013892 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 25 Jan 2011 14:54:33 -0700
Subject: [PATCH] UBUNTU: xen unified block-device I/O interface back end can orphan devices, CVE-2010-3699

CVE-2010-3699
BugLink: http://bugs.launchpad.net/bugs/???

Excerpted from http://rhn.redhat.com/errata/RHSA-2011-0004.html:

"A flaw was found in the Xenbus code for the unified block-device I/O
interface back end. A privileged guest user could use this flaw to cause a
denial of service on the host system running the Xen hypervisor."

Most of this patch was developed from http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/59f097ef181b.
A careful comparision of the patch and the actual contents of http://xenbits.xensource.com/linux-2.6.18-xen.hg
showed that part of the original patch introduced a regression as can be seen from the Mercurial log.
I've not implemented that piece of the patch.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 .../xen/patchset/020-xen-CVE-2010-3699.patch       |  187 ++++++++++++++++++++
 1 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch

diff --git a/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch b/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
new file mode 100644
index 0000000..3ada6fa
--- /dev/null
+++ b/debian/binary-custom.d/xen/patchset/020-xen-CVE-2010-3699.patch
@@ -0,0 +1,187 @@ 
+diff -Nurp custom-source-xen.orig/drivers/xen/blkback/xenbus.c custom-source-xen/drivers/xen/blkback/xenbus.c
+--- custom-source-xen.orig/drivers/xen/blkback/xenbus.c	2011-01-26 18:31:29.866234416 +0100
++++ custom-source-xen/drivers/xen/blkback/xenbus.c	2011-01-26 18:29:26.690234371 +0100
+@@ -362,6 +362,11 @@ static void frontend_changed(struct xenb
+ 		if (dev->state == XenbusStateConnected)
+ 			break;
+ 
++		/* Enforce precondition before potential leak point.
++		 * blkif_disconnect() is idempotent.
++		 */
++		blkif_disconnect(be->blkif);
++
+ 		err = connect_ring(be);
+ 		if (err)
+ 			break;
+@@ -379,6 +384,7 @@ static void frontend_changed(struct xenb
+ 			break;
+ 		/* fall through if not online */
+ 	case XenbusStateUnknown:
++		/* implies blkif_disconnect() via blkback_remove() */
+ 		device_unregister(&dev->dev);
+ 		break;
+ 
+diff -Nurp custom-source-xen.orig/drivers/xen/blktap/common.h custom-source-xen/drivers/xen/blktap/common.h
+--- custom-source-xen.orig/drivers/xen/blktap/common.h	2011-01-26 18:29:25.974232874 +0100
++++ custom-source-xen/drivers/xen/blktap/common.h	2011-01-26 18:32:10.622236863 +0100
+@@ -89,6 +89,7 @@ typedef struct blkif_st {
+ 
+ blkif_t *tap_alloc_blkif(domid_t domid);
+ void tap_blkif_free(blkif_t *blkif);
++void tap_blkif_kmem_cache_free(blkif_t *blkif);
+ int tap_blkif_map(blkif_t *blkif, unsigned long shared_page, 
+ 		  unsigned int evtchn);
+ void tap_blkif_unmap(blkif_t *blkif);
+diff -Nurp custom-source-xen.orig/drivers/xen/blktap/interface.c custom-source-xen/drivers/xen/blktap/interface.c
+--- custom-source-xen.orig/drivers/xen/blktap/interface.c	2011-01-26 18:31:29.882243505 +0100
++++ custom-source-xen/drivers/xen/blktap/interface.c	2011-01-26 18:29:26.690234371 +0100
+@@ -162,8 +162,15 @@ void tap_blkif_free(blkif_t *blkif)
+ {
+ 	atomic_dec(&blkif->refcnt);
+ 	wait_event(blkif->waiting_to_free, atomic_read(&blkif->refcnt) == 0);
++	atomic_inc(&blkif->refcnt);
+ 
+ 	tap_blkif_unmap(blkif);
++}
++
++void tap_blkif_kmem_cache_free(blkif_t *blkif)
++{
++	if (!atomic_dec_and_test(&blkif->refcnt))
++		BUG();
+ 	kmem_cache_free(blkif_cachep, blkif);
+ }
+ 
+diff -Nurp custom-source-xen.orig/drivers/xen/blktap/xenbus.c custom-source-xen/drivers/xen/blktap/xenbus.c
+--- custom-source-xen.orig/drivers/xen/blktap/xenbus.c	2011-01-26 18:31:29.882243505 +0100
++++ custom-source-xen/drivers/xen/blktap/xenbus.c	2011-01-26 18:29:26.690234371 +0100
+@@ -183,6 +183,7 @@ static int blktap_remove(struct xenbus_d
+ 			kthread_stop(be->blkif->xenblkd);
+ 		signal_tapdisk(be->blkif->dev_num);
+ 		tap_blkif_free(be->blkif);
++		tap_blkif_kmem_cache_free(be->blkif);
+ 		be->blkif = NULL;
+ 	}
+ 	kfree(be);
+@@ -326,6 +327,18 @@ static void tap_backend_changed(struct x
+ 	tap_update_blkif_status(be->blkif);
+ }
+ 
++
++static void tap_blkif_disconnect(blkif_t *blkif)
++{
++	if (blkif->xenblkd) {
++		kthread_stop(blkif->xenblkd);
++		blkif->xenblkd = NULL;
++	}
++
++	/* idempotent */
++	tap_blkif_free(blkif);
++}
++
+ /**
+  * Callback received when the frontend's state changes.
+  */
+@@ -354,6 +367,11 @@ static void tap_frontend_changed(struct 
+ 		if (dev->state == XenbusStateConnected)
+ 			break;
+ 
++		/* Enforce precondition before potential leak point.
++		 * tap_blkif_disconnect() is idempotent.
++		 */
++		tap_blkif_disconnect(be->blkif);
++
+ 		err = connect_ring(be);
+ 		if (err)
+ 			break;
+@@ -361,10 +379,7 @@ static void tap_frontend_changed(struct 
+ 		break;
+ 
+ 	case XenbusStateClosing:
+-		if (be->blkif->xenblkd) {
+-			kthread_stop(be->blkif->xenblkd);
+-			be->blkif->xenblkd = NULL;
+-		}
++		tap_blkif_disconnect(be->blkif);
+ 		xenbus_switch_state(dev, XenbusStateClosing);
+ 		break;
+ 
+@@ -374,6 +389,9 @@ static void tap_frontend_changed(struct 
+ 			break;
+ 		/* fall through if not online */
+ 	case XenbusStateUnknown:
++		/* Implies the effects of blkif_disconnect() via
++		 * blktap_remove().
++		 */
+ 		device_unregister(&dev->dev);
+ 		break;
+ 
+diff -Nurp custom-source-xen.orig/drivers/xen/netback/xenbus.c custom-source-xen/drivers/xen/netback/xenbus.c
+--- custom-source-xen.orig/drivers/xen/netback/xenbus.c	2011-01-26 18:31:29.882243505 +0100
++++ custom-source-xen/drivers/xen/netback/xenbus.c	2011-01-26 18:29:26.690234371 +0100
+@@ -32,6 +32,7 @@
+ static int connect_rings(struct backend_info *);
+ static void connect(struct backend_info *);
+ static void backend_create_netif(struct backend_info *be);
++static void netback_disconnect(struct device *);
+ 
+ static int netback_remove(struct xenbus_device *dev)
+ {
+@@ -39,15 +40,22 @@ static int netback_remove(struct xenbus_
+ 
+ 	netback_remove_accelerators(be, dev);
+ 
+-	if (be->netif) {
+-		netif_disconnect(be->netif);
+-		be->netif = NULL;
+-	}
++	netback_disconnect(&dev->dev);
+ 	kfree(be);
+ 	dev->dev.driver_data = NULL;
+ 	return 0;
+ }
+ 
++static void netback_disconnect(struct device *xbdev_dev)
++{
++        struct backend_info *be = xbdev_dev->driver_data;
++ 
++        if (be->netif) {
++                kobject_uevent(&xbdev_dev->kobj, KOBJ_OFFLINE);
++                netif_disconnect(be->netif);
++                be->netif = NULL;
++        }
++}
+ 
+ /**
+  * Entry point to this code when a new device is created.  Allocate the basic
+@@ -225,16 +233,17 @@ static void frontend_changed(struct xenb
+ 		break;
+ 
+ 	case XenbusStateConnected:
++		if (dev->state == XenbusStateConnected)
++			break;
++
++		/* backend_create_netif() is idempotent */
+ 		backend_create_netif(be);
+ 		if (be->netif)
+ 			connect(be);
+ 		break;
+ 
+ 	case XenbusStateClosing:
+-		if (be->netif) {
+-			netif_disconnect(be->netif);
+-			be->netif = NULL;
+-		}
++		netback_disconnect(&dev->dev);
+ 		xenbus_switch_state(dev, XenbusStateClosing);
+ 		break;
+ 
+@@ -244,8 +253,7 @@ static void frontend_changed(struct xenb
+ 			break;
+ 		/* fall through if not online */
+ 	case XenbusStateUnknown:
+-		if (be->netif != NULL)
+-			kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
++		/* implies netback_disconnect() via netback_remove() */
+ 		device_unregister(&dev->dev);
+ 		break;
+ 
-- 
1.7.0.4