* [PATCH] driver core : Fix use after free of dev->parent in device_shutdown
@ 2013-09-24 21:39 Benson Leung
2013-09-24 22:10 ` Greg KH
2013-09-25 1:14 ` Ming Lei
0 siblings, 2 replies; 4+ messages in thread
From: Benson Leung @ 2013-09-24 21:39 UTC (permalink / raw)
To: gregkh, ming.lei, linux-kernel; +Cc: bleung, olofj, stable
The put_device(dev) at the bottom of the loop of device_shutdown
may result in the dev being cleaned up. In device_create_release,
the dev is kfreed.
However, device_shutdown attempts to use the dev pointer again after
put_device by referring to dev->parent.
Copy the parent pointer instead to avoid this condition.
This bug was found on Chromium OS's chromeos-3.8, which is based on v3.8.11.
See bug report : https://code.google.com/p/chromium/issues/detail?id=297842
This can easily be reproduced when shutting down with
hidraw devices that report battery condition.
Two examples are the HP Bluetooth Mouse X4000b and the Apple Magic Mouse.
For example, with the magic mouse :
The dev in question is "hidraw0"
dev->parent is "magicmouse"
In the course of the shutdown for this device, the input event cleanup calls
a put on hidraw0, decrementing its reference count.
When we finally get to put_device(dev) in device_shutdown, kobject_cleanup
is called and device_create_release does kfree(dev).
dev->parent is no longer valid, and we may crash in
put_device(dev->parent).
This change should be applied on any kernel with this change :
d1c6c030fcec6f860d9bb6c632a3ebe62e28440b
Cc: stable@vger.kernel.org
Signed-off-by: Benson Leung <bleung@chromium.org>
---
drivers/base/core.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index c7cfadc..34bd546 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2026,6 +2026,8 @@ void device_shutdown(void)
* devices offline, even as the system is shutting down.
*/
while (!list_empty(&devices_kset->list)) {
+ struct device *parent;
+
dev = list_entry(devices_kset->list.prev, struct device,
kobj.entry);
@@ -2034,7 +2036,8 @@ void device_shutdown(void)
* prevent it from being freed because parent's
* lock is to be held
*/
- get_device(dev->parent);
+ parent = dev->parent;
+ get_device(parent);
get_device(dev);
/*
* Make sure the device is off the kset list, in the
@@ -2044,8 +2047,8 @@ void device_shutdown(void)
spin_unlock(&devices_kset->list_lock);
/* hold lock to avoid race with probe/release */
- if (dev->parent)
- device_lock(dev->parent);
+ if (parent)
+ device_lock(parent);
device_lock(dev);
/* Don't allow any more runtime suspends */
@@ -2063,11 +2066,11 @@ void device_shutdown(void)
}
device_unlock(dev);
- if (dev->parent)
- device_unlock(dev->parent);
+ if (parent)
+ device_unlock(parent);
put_device(dev);
- put_device(dev->parent);
+ put_device(parent);
spin_lock(&devices_kset->list_lock);
}
--
1.8.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] driver core : Fix use after free of dev->parent in device_shutdown
2013-09-24 21:39 [PATCH] driver core : Fix use after free of dev->parent in device_shutdown Benson Leung
@ 2013-09-24 22:10 ` Greg KH
2013-09-25 1:14 ` Ming Lei
1 sibling, 0 replies; 4+ messages in thread
From: Greg KH @ 2013-09-24 22:10 UTC (permalink / raw)
To: Benson Leung; +Cc: ming.lei, linux-kernel, olofj, stable
On Tue, Sep 24, 2013 at 02:39:08PM -0700, Benson Leung wrote:
> The put_device(dev) at the bottom of the loop of device_shutdown
> may result in the dev being cleaned up. In device_create_release,
> the dev is kfreed.
>
> However, device_shutdown attempts to use the dev pointer again after
> put_device by referring to dev->parent.
>
> Copy the parent pointer instead to avoid this condition.
>
> This bug was found on Chromium OS's chromeos-3.8, which is based on v3.8.11.
> See bug report : https://code.google.com/p/chromium/issues/detail?id=297842
> This can easily be reproduced when shutting down with
> hidraw devices that report battery condition.
> Two examples are the HP Bluetooth Mouse X4000b and the Apple Magic Mouse.
> For example, with the magic mouse :
> The dev in question is "hidraw0"
> dev->parent is "magicmouse"
>
> In the course of the shutdown for this device, the input event cleanup calls
> a put on hidraw0, decrementing its reference count.
> When we finally get to put_device(dev) in device_shutdown, kobject_cleanup
> is called and device_create_release does kfree(dev).
> dev->parent is no longer valid, and we may crash in
> put_device(dev->parent).
>
> This change should be applied on any kernel with this change :
> d1c6c030fcec6f860d9bb6c632a3ebe62e28440b
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Benson Leung <bleung@chromium.org>
The patch makes sense, but I wonder why no one has ever seen this
problem before.
Odd...
thanks for the fix, I'll queue it up for 3.12-final soon.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] driver core : Fix use after free of dev->parent in device_shutdown
2013-09-24 21:39 [PATCH] driver core : Fix use after free of dev->parent in device_shutdown Benson Leung
2013-09-24 22:10 ` Greg KH
@ 2013-09-25 1:14 ` Ming Lei
2013-09-25 3:07 ` Benson Leung
1 sibling, 1 reply; 4+ messages in thread
From: Ming Lei @ 2013-09-25 1:14 UTC (permalink / raw)
To: Benson Leung; +Cc: Greg Kroah-Hartman, Linux Kernel Mailing List, olofj, stable
On Wed, Sep 25, 2013 at 5:39 AM, Benson Leung <bleung@chromium.org> wrote:
> The put_device(dev) at the bottom of the loop of device_shutdown
> may result in the dev being cleaned up. In device_create_release,
> the dev is kfreed.
>
> However, device_shutdown attempts to use the dev pointer again after
> put_device by referring to dev->parent.
>
> Copy the parent pointer instead to avoid this condition.
Right.
>
> This bug was found on Chromium OS's chromeos-3.8, which is based on v3.8.11.
> See bug report : https://code.google.com/p/chromium/issues/detail?id=297842
> This can easily be reproduced when shutting down with
> hidraw devices that report battery condition.
> Two examples are the HP Bluetooth Mouse X4000b and the Apple Magic Mouse.
> For example, with the magic mouse :
> The dev in question is "hidraw0"
> dev->parent is "magicmouse"
>
> In the course of the shutdown for this device, the input event cleanup calls
> a put on hidraw0, decrementing its reference count.
> When we finally get to put_device(dev) in device_shutdown, kobject_cleanup
> is called and device_create_release does kfree(dev).
> dev->parent is no longer valid, and we may crash in
> put_device(dev->parent).
>
> This change should be applied on any kernel with this change :
> d1c6c030fcec6f860d9bb6c632a3ebe62e28440b
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/base/core.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index c7cfadc..34bd546 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2026,6 +2026,8 @@ void device_shutdown(void)
> * devices offline, even as the system is shutting down.
> */
> while (!list_empty(&devices_kset->list)) {
> + struct device *parent;
> +
> dev = list_entry(devices_kset->list.prev, struct device,
> kobj.entry);
>
> @@ -2034,7 +2036,8 @@ void device_shutdown(void)
> * prevent it from being freed because parent's
> * lock is to be held
> */
> - get_device(dev->parent);
> + parent = dev->parent;
> + get_device(parent);
It is better to save one line by below:
parent = get_device(dev->parent);
> get_device(dev);
> /*
> * Make sure the device is off the kset list, in the
> @@ -2044,8 +2047,8 @@ void device_shutdown(void)
> spin_unlock(&devices_kset->list_lock);
>
> /* hold lock to avoid race with probe/release */
> - if (dev->parent)
> - device_lock(dev->parent);
> + if (parent)
> + device_lock(parent);
> device_lock(dev);
>
> /* Don't allow any more runtime suspends */
> @@ -2063,11 +2066,11 @@ void device_shutdown(void)
> }
>
> device_unlock(dev);
> - if (dev->parent)
> - device_unlock(dev->parent);
> + if (parent)
> + device_unlock(parent);
>
> put_device(dev);
> - put_device(dev->parent);
> + put_device(parent);
>
> spin_lock(&devices_kset->list_lock);
> }
Reviewed-by: Ming Lei <ming.lei@canonical.com>
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] driver core : Fix use after free of dev->parent in device_shutdown
2013-09-25 1:14 ` Ming Lei
@ 2013-09-25 3:07 ` Benson Leung
0 siblings, 0 replies; 4+ messages in thread
From: Benson Leung @ 2013-09-25 3:07 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, Linux Kernel Mailing List, Olof Johansson, stable
On Tue, Sep 24, 2013 at 6:14 PM, Ming Lei <ming.lei@canonical.com> wrote:
> It is better to save one line by below:
>
> parent = get_device(dev->parent);
>
Done.
>
> Reviewed-by: Ming Lei <ming.lei@canonical.com>
Thank you!
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-25 3:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-24 21:39 [PATCH] driver core : Fix use after free of dev->parent in device_shutdown Benson Leung
2013-09-24 22:10 ` Greg KH
2013-09-25 1:14 ` Ming Lei
2013-09-25 3:07 ` Benson Leung
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.