lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Apr 2019 23:44:00 +0800 From: Muchun Song <smuchun@...il.com> To: Benjamin Herrenschmidt <benh@...nel.crashing.org> Cc: gregkh@...uxfoundation.org, rafael@...nel.org, linux-kernel <linux-kernel@...r.kernel.org>, zhaowuyun@...gtech.com Subject: Re: [PATCH] driver core: Fix use-after-free and double free on glue directory Hi Cheers, Benjamin Herrenschmidt <benh@...nel.crashing.org> 于2019年4月25日周四 下午5:24写道: > > On Tue, 2019-04-23 at 22:32 +0800, Muchun Song wrote: > > There is a race condition between removing glue directory and adding a new > > device under the glue directory. It can be reproduced in following test: > > > > .../... > > > In order to avoid this happening, we we should not call kobject_del() on > > path2 when the reference count of glue_dir is greater than 1. So we add a > > conditional statement to fix it. > > Good catch ! However I'm not completely happy about the fix you > propose. > > I find relying on the object count for such decisions rather fragile as > it could be taken temporarily for other reasons, couldn't it ? In which > case we would just fail... It could be taken temporarily for other reasons, what reasons? I also can not figure out which case could result in this. > > Ideally, the looking up of the glue dir and creation of its child > should be protected by the same lock instance (the gdp_mutex in that > case). > > That might require a bit of shuffling around though. > > Greg, thoughts ? This whole gluedir business is annoyingly racy still. > > My gut feeling is that the "right fix" is to ensure the lookup of the > glue dir and creation of the child object(s) are done under a single > instance of gdp_mutex so we never see a stale "empty" but still > poentially used glue dir around. I agree with you that the looking up of the glue dir and creation of its child should be protected by the same lock of gdp_mutex. So, do you agree with the fix of the following code snippet? --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1740,8 +1740,11 @@ class_dir_create_and_add(struct class *class, struct kobject *parent_kobj) static DEFINE_MUTEX(gdp_mutex); static struct kobject *get_device_parent(struct device *dev, - struct device *parent) + struct device *parent, + bool *locked) { + *locked = false; + if (dev->class) { struct kobject *kobj = NULL; struct kobject *parent_kobj; @@ -1779,7 +1782,7 @@ static struct kobject *get_device_parent(struct device *dev, } spin_unlock(&dev->class->p->glue_dirs.list_lock); if (kobj) { - mutex_unlock(&gdp_mutex); + *locked = true; return kobj; } @@ -2007,6 +2010,7 @@ int device_add(struct device *dev) struct class_interface *class_intf; int error = -EINVAL; struct kobject *glue_dir = NULL; + bool locked; dev = get_device(dev); if (!dev) @@ -2040,7 +2044,7 @@ int device_add(struct device *dev) pr_debug("device: '%s': %s\n", dev_name(dev), __func__); parent = get_device(dev->parent); - kobj = get_device_parent(dev, parent); + kobj = get_device_parent(dev, parent, &locked); if (IS_ERR(kobj)) { error = PTR_ERR(kobj); goto parent_error; @@ -2057,9 +2061,14 @@ int device_add(struct device *dev) error = kobject_add(&dev->kobj, dev->kobj.parent, NULL); if (error) { glue_dir = get_glue_dir(dev); + if (locked) + mutex_unlock(&gdp_mutex); goto Error; } + if (locked) + mutex_unlock(&gdp_mutex); + /* notify platform of device entry */ error = device_platform_notify(dev, KOBJ_ADD); if (error) Yours Muchun
Powered by blists - more mailing lists