Re: 2.6.13-rc4 use after free in class_device_attr_show

[Maneesh Soni <maneesh@in.ibm.com>]

On Wed, Aug 10, 2005 at 03:35:53PM -0700, Greg KH wrote:
> On Wed, Aug 10, 2005 at 03:36:36PM +0530, Maneesh Soni wrote:
> > On Wed, Aug 10, 2005 at 04:26:51PM +1000, Keith Owens wrote:
> > > FYI, the intermittent free after use in sysfs is still there in
> > > 2.6.13-rc6.
> > > 
> > 
> > The race condition is known here. It is some thing in the upper layer. 
> > In this case "driver/base/class.c" which frees the kobject's attributes 
> > even if there are live references to kobject.
> > 
> > 
> > open sysfs file				unregister class device
> > sysfs_open_file()			class_device_del()
> >   -> takes a ref on kobject		  -> kfree attribute struct
> >      -> accesses attributes		  -> kobject_del()
> > 					      -> kref_put()	
> > close sysfs file				  
> > sysfs_release()				    
> >   -> acesses attributes using s_element
> >   -> drops ref to kobject
> > 
> > Solution could be either we have reference counting for attributes also
> > or keep attributes alive till the last reference to the kobject. Both these
> > needs changes in the driver core.
> > 
> > Greg, will the following patch make sense? This postpones the kfree() of
> > devt_attr till class_dev_release() is called. 
> 
> Yes, that patch looks good, if you fix up the space vs. tabs issue :)
> 

yuk.. sorry for that.. I corrected that now.

> But will that really fix this race?  I was under the impression the oops
> didn't come from trying to access the devt_attr, but the sysfs s_element
> pointer?

I am not able to recreate the race with or without the patch though. As per
the stack traces, and the debug messages from antother thread also,
I could see that it is use after free for attribute structure. s_element
is a pointer to the attribute structure in case of sysfs "files". Most of
the times this attribute structure is not allocated/freed dynamically unlike
/sys/class/vc/vcs*/dev, so we don't see any crashes.


> > Please check this patch out, if this helps or not.
> 
> I'd be interested in seeing if this fixes it.
> 

I just cc-ed others also who reported similar oops.

Thanks
Maneesh




o following patch moves the code to free devt_attr from class_device_del()
  to class_dev_release() which is called after the last reference to the
  corresponding kobject() is gone. This allows to keep the devt_attr 
  alive while the corresponding sysfs file is open.

Signed-off-by: Maneesh Soni <maneesh@in.ibm.com>
---

 linux-2.6.13-rc6-maneesh/drivers/base/class.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff -puN drivers/base/class.c~fix-class-attributes-race drivers/base/class.c
--- linux-2.6.13-rc6/drivers/base/class.c~fix-class-attributes-race	2005-08-10 12:35:06.000000000 +0530
+++ linux-2.6.13-rc6-maneesh/drivers/base/class.c	2005-08-11 09:27:17.202081088 +0530
@@ -299,6 +299,11 @@ static void class_dev_release(struct kob
 
 	pr_debug("device class '%s': release.\n", cd->class_id);
 
+	if (cd->devt_attr) {
+		kfree(cd->devt_attr);
+		cd->devt_attr = NULL;
+	}
+
 	if (cls->release)
 		cls->release(cd);
 	else {
@@ -591,11 +596,10 @@ void class_device_del(struct class_devic
 
 	if (class_dev->dev)
 		sysfs_remove_link(&class_dev->kobj, "device");
-	if (class_dev->devt_attr) {
-		class_device_remove_file(class_dev, class_dev->devt_attr);
-		kfree(class_dev->devt_attr);
-		class_dev->devt_attr = NULL;
-	}
+
+	if (class_dev->devt_attr)
+                class_device_remove_file(class_dev, class_dev->devt_attr);
+
 	class_device_remove_attrs(class_dev);
 
 	kobject_hotplug(&class_dev->kobj, KOBJ_REMOVE);
_

-- 
Maneesh Soni
Linux Technology Center, 
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990