[PATCH] drivers-core: nullify private pointer on device-release

[PATCH] drivers-core: nullify private pointer on device-release

[Guennadi Liakhovetski]

Device structures can be reused over multiple device_add / device_release 
cycles. As the private pointer in that struct is checked for NULL, it has 
to be nullified after freeing the private data. Failure to do so causes a 
use after free bug.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6bee6af..3c9f449 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -122,6 +122,7 @@ static void device_release(struct kobject *kobj)
 		WARN(1, KERN_ERR "Device '%s' does not have a release() "
 			"function, it is broken and must be fixed.\n",
 			dev_name(dev));
+	dev->p = NULL;
 	kfree(p);
 }
 

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Greg KH]

On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> Device structures can be reused over multiple device_add / device_release 
> cycles.

They shouldn't be as they should be dynamic, not static.

What device is having this problem?

thanks,

greg k-h

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Guennadi Liakhovetski]

On Thu, 1 Oct 2009, Greg KH wrote:

> On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > Device structures can be reused over multiple device_add / device_release 
> > cycles.
> 
> They shouldn't be as they should be dynamic, not static.

Should they? I'm pretty sure this is not the first time this comes up - 
there are several drivers and / or subsystems, that re-use driver objects. 
But finding in mail archives wouldn't be very easy. And it worked until 
now - why should we break it?

> What device is having this problem?

My problem case is the soc-camera framework. There device struct is 
embedded into the video client object, which are kept as long as the 
driver is loaded.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Greg KH]

On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:
> On Thu, 1 Oct 2009, Greg KH wrote:
> 
> > On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > > Device structures can be reused over multiple device_add / device_release 
> > > cycles.
> > 
> > They shouldn't be as they should be dynamic, not static.
> 
> Should they? I'm pretty sure this is not the first time this comes up - 
> there are several drivers and / or subsystems, that re-use driver objects. 

Then those drivers and subsystems should be fixed, as that is incorrect.

> But finding in mail archives wouldn't be very easy. And it worked until 
> now - why should we break it?

I would argue that this code was always broken.
When did this problem show up for you?

> > What device is having this problem?
> 
> My problem case is the soc-camera framework. There device struct is 
> embedded into the video client object, which are kept as long as the 
> driver is loaded.

struct device is a dynamic structure, it is supposed to be able to be
freed when the last reference goes away.  Static struct device usage is
wrong.  It sounds like the video client object code is incorrect, please
fix it.

thanks,

greg k-h

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Kay Sievers]

On Thu, Oct 1, 2009 at 16:15, Greg KH <greg@kroah.com> wrote:
> On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:

>> My problem case is the soc-camera framework. There device struct is
>> embedded into the video client object, which are kept as long as the
>> driver is loaded.
>
> struct device is a dynamic structure, it is supposed to be able to be
> freed when the last reference goes away.  Static struct device usage is
> wrong.  It sounds like the video client object code is incorrect, please
> fix it.

Exactly. If you keep your "object" around for forever, you must never
embed a struct device but only a pointer to one that is managed by the
core. Embedding a struct device defers lifetime management of the
enclosing object to the driver core reference counts.

You can not have both, you have to decide to let your objects be
managed by the driver core reference, or just point to a dynamic
driver core object. All driver core objects are always dynamic,
everything else just seem to work, but are not expected to work
reliably.

Thanks,
Kay

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Guennadi Liakhovetski]

On Thu, 1 Oct 2009, Greg KH wrote:

> On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:
> > On Thu, 1 Oct 2009, Greg KH wrote:
> > 
> > > On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > > > Device structures can be reused over multiple device_add / device_release 
> > > > cycles.
> > > 
> > > They shouldn't be as they should be dynamic, not static.
> > 
> > Should they? I'm pretty sure this is not the first time this comes up - 
> > there are several drivers and / or subsystems, that re-use driver objects. 
> 
> Then those drivers and subsystems should be fixed, as that is incorrect.
> 
> > But finding in mail archives wouldn't be very easy. And it worked until 
> > now - why should we break it?
> 
> I would argue that this code was always broken.
> When did this problem show up for you?

Since commit b4028437876866aba4747a655ede00f892089e14

> > > What device is having this problem?
> > 
> > My problem case is the soc-camera framework. There device struct is 
> > embedded into the video client object, which are kept as long as the 
> > driver is loaded.
> 
> struct device is a dynamic structure, it is supposed to be able to be
> freed when the last reference goes away.  Static struct device usage is
> wrong.  It sounds like the video client object code is incorrect, please
> fix it.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Greg KH]

On Mon, Oct 05, 2009 at 10:11:50AM +0200, Guennadi Liakhovetski wrote:
> On Thu, 1 Oct 2009, Greg KH wrote:
> 
> > On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:
> > > On Thu, 1 Oct 2009, Greg KH wrote:
> > > 
> > > > On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > > > > Device structures can be reused over multiple device_add / device_release 
> > > > > cycles.
> > > > 
> > > > They shouldn't be as they should be dynamic, not static.
> > > 
> > > Should they? I'm pretty sure this is not the first time this comes up - 
> > > there are several drivers and / or subsystems, that re-use driver objects. 
> > 
> > Then those drivers and subsystems should be fixed, as that is incorrect.
> > 
> > > But finding in mail archives wouldn't be very easy. And it worked until 
> > > now - why should we break it?
> > 
> > I would argue that this code was always broken.
> > When did this problem show up for you?
> 
> Since commit b4028437876866aba4747a655ede00f892089e14

Again, which driver/devices are having this problem?  The patch
referenced above had been in linux-next for almost 6 months with no
reported problems, so this is news to me :)

thanks,

greg k-h

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Guennadi Liakhovetski]

On Mon, 5 Oct 2009, Greg KH wrote:

> On Mon, Oct 05, 2009 at 10:11:50AM +0200, Guennadi Liakhovetski wrote:
> > On Thu, 1 Oct 2009, Greg KH wrote:
> > 
> > > On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:
> > > > On Thu, 1 Oct 2009, Greg KH wrote:
> > > > 
> > > > > On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > > > > > Device structures can be reused over multiple device_add / device_release 
> > > > > > cycles.
> > > > > 
> > > > > They shouldn't be as they should be dynamic, not static.
> > > > 
> > > > Should they? I'm pretty sure this is not the first time this comes up - 
> > > > there are several drivers and / or subsystems, that re-use driver objects. 
> > > 
> > > Then those drivers and subsystems should be fixed, as that is incorrect.
> > > 
> > > > But finding in mail archives wouldn't be very easy. And it worked until 
> > > > now - why should we break it?
> > > 
> > > I would argue that this code was always broken.
> > > When did this problem show up for you?
> > 
> > Since commit b4028437876866aba4747a655ede00f892089e14
> 
> Again, which driver/devices are having this problem?

Quoting my previous reply in this thread:

> > What device is having this problem?
> 
> My problem case is the soc-camera framework. There device struct is 
> embedded into the video client object, which are kept as long as the 
> driver is loaded.

</quote>

> The patch
> referenced above had been in linux-next for almost 6 months with no
> reported problems, so this is news to me :)

Hm, really 6 months in linux-next? That surprises me too, that I didn't 
notice it until now. But in any case, the aforementioned commit introduced 
a regression. If you disagree, that it has to be fixed centrally as per 
proposed patch, no problem, I'm pushing a patch tonight, that will fix 
this for soc-camera. No idea about other drivers.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] drivers-core: nullify private pointer on device-release

[Greg KH]

On Mon, Oct 05, 2009 at 06:14:37PM +0200, Guennadi Liakhovetski wrote:
> On Mon, 5 Oct 2009, Greg KH wrote:
> 
> > On Mon, Oct 05, 2009 at 10:11:50AM +0200, Guennadi Liakhovetski wrote:
> > > On Thu, 1 Oct 2009, Greg KH wrote:
> > > 
> > > > On Thu, Oct 01, 2009 at 03:45:56PM +0200, Guennadi Liakhovetski wrote:
> > > > > On Thu, 1 Oct 2009, Greg KH wrote:
> > > > > 
> > > > > > On Thu, Oct 01, 2009 at 10:02:08AM +0200, Guennadi Liakhovetski wrote:
> > > > > > > Device structures can be reused over multiple device_add / device_release 
> > > > > > > cycles.
> > > > > > 
> > > > > > They shouldn't be as they should be dynamic, not static.
> > > > > 
> > > > > Should they? I'm pretty sure this is not the first time this comes up - 
> > > > > there are several drivers and / or subsystems, that re-use driver objects. 
> > > > 
> > > > Then those drivers and subsystems should be fixed, as that is incorrect.
> > > > 
> > > > > But finding in mail archives wouldn't be very easy. And it worked until 
> > > > > now - why should we break it?
> > > > 
> > > > I would argue that this code was always broken.
> > > > When did this problem show up for you?
> > > 
> > > Since commit b4028437876866aba4747a655ede00f892089e14
> > 
> > Again, which driver/devices are having this problem?
> 
> Quoting my previous reply in this thread:
> 
> > > What device is having this problem?
> > 
> > My problem case is the soc-camera framework. There device struct is 
> > embedded into the video client object, which are kept as long as the 
> > driver is loaded.
> 
> </quote>

Ok, then that code needs to be fixed :)

> > The patch
> > referenced above had been in linux-next for almost 6 months with no
> > reported problems, so this is news to me :)
> 
> Hm, really 6 months in linux-next? That surprises me too, that I didn't 
> notice it until now. But in any case, the aforementioned commit introduced 
> a regression. If you disagree, that it has to be fixed centrally as per 
> proposed patch, no problem, I'm pushing a patch tonight, that will fix 
> this for soc-camera. No idea about other drivers.

Great, glad to see this being fixed where it should be.  As no one else
has reported a problem, I'm guessing that this is the only incorrect
code.

thanks,

greg k-h