[PATCH] Initialize pointer on request_firmware

[PATCH] Initialize pointer on request_firmware

[Lucas C. Villa Real]

Hello, folks,

I've seen some kernel oopses when suspending my machine. The problem comes from isight_firmware, which assumes that, on error, a call to request_firmware() will initialize the provided pointer to the firmware image to NULL.

The patch below fixes the isight_firmware side of the problem and also ensures that request_firmware() always sets the pointer to NULL on such cases (it currently does that for all except one situation).

Signed-off-by: Lucas C. Villa Real <lucasvr@gobolinux.org>

--- linux-3.0.4/drivers/base/firmware_class.c.orig	2011-09-21 21:03:01.000000000 -0300
+++ linux-3.0.4/drivers/base/firmware_class.c	2011-09-21 21:03:13.000000000 -0300
@@ -523,6 +523,7 @@ static int _request_firmware(const struc
 
 	if (WARN_ON(usermodehelper_is_disabled())) {
 		dev_err(device, "firmware: %s will not be loaded\n", name);
+		*firmware_p = NULL;
 		return -EBUSY;
 	}
 
--- linux-3.0.4/drivers/usb/misc/isight_firmware.c.orig	2011-09-21 20:47:39.000000000 -0300
+++ linux-3.0.4/drivers/usb/misc/isight_firmware.c	2011-09-21 20:47:46.000000000 -0300
@@ -39,7 +39,7 @@ static int isight_firmware_load(struct u
 {
 	struct usb_device *dev = interface_to_usbdev(intf);
 	int llen, len, req, ret = 0;
-	const struct firmware *firmware;
+	const struct firmware *firmware = NULL;
 	unsigned char *buf = kmalloc(50, GFP_KERNEL);
 	unsigned char data[4];
 	const u8 *ptr;

Re: [PATCH] Initialize pointer on request_firmware

[Borislav Petkov]

On Wed, Sep 21, 2011 at 11:55:15PM -0300, Lucas C. Villa Real wrote:
> Hello, folks,
> 
> I've seen some kernel oopses when suspending my machine. The problem comes from isight_firmware, which assumes that, on error, a call to request_firmware() will initialize the provided pointer to the firmware image to NULL.
> 
> The patch below fixes the isight_firmware side of the problem and also ensures that request_firmware() always sets the pointer to NULL on such cases (it currently does that for all except one situation).
> 
> Signed-off-by: Lucas C. Villa Real <lucasvr@gobolinux.org>
> 
> --- linux-3.0.4/drivers/base/firmware_class.c.orig	2011-09-21 21:03:01.000000000 -0300
> +++ linux-3.0.4/drivers/base/firmware_class.c	2011-09-21 21:03:13.000000000 -0300
> @@ -523,6 +523,7 @@ static int _request_firmware(const struc
>  
>  	if (WARN_ON(usermodehelper_is_disabled())) {
>  		dev_err(device, "firmware: %s will not be loaded\n", name);
> +		*firmware_p = NULL;
>  		return -EBUSY;
>  	}

Looks like f45f3c1f3f616 needs backporting to stable, if it hasn't
happened yet. Oh, and then there's caca9510ff4e5 too which adds this
exit path to the goto out label as the rest of the function.

Adding some more people to Cc.

>  
> --- linux-3.0.4/drivers/usb/misc/isight_firmware.c.orig	2011-09-21 20:47:39.000000000 -0300
> +++ linux-3.0.4/drivers/usb/misc/isight_firmware.c	2011-09-21 20:47:46.000000000 -0300
> @@ -39,7 +39,7 @@ static int isight_firmware_load(struct u
>  {
>  	struct usb_device *dev = interface_to_usbdev(intf);
>  	int llen, len, req, ret = 0;
> -	const struct firmware *firmware;
> +	const struct firmware *firmware = NULL;
>  	unsigned char *buf = kmalloc(50, GFP_KERNEL);
>  	unsigned char data[4];
>  	const u8 *ptr;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] Initialize pointer on request_firmware

[Greg KH]

On Thu, Sep 22, 2011 at 11:23:47AM +0200, Borislav Petkov wrote:
> On Wed, Sep 21, 2011 at 11:55:15PM -0300, Lucas C. Villa Real wrote:
> > Hello, folks,
> > 
> > I've seen some kernel oopses when suspending my machine. The problem comes from isight_firmware, which assumes that, on error, a call to request_firmware() will initialize the provided pointer to the firmware image to NULL.
> > 
> > The patch below fixes the isight_firmware side of the problem and also ensures that request_firmware() always sets the pointer to NULL on such cases (it currently does that for all except one situation).
> > 
> > Signed-off-by: Lucas C. Villa Real <lucasvr@gobolinux.org>
> > 
> > --- linux-3.0.4/drivers/base/firmware_class.c.orig	2011-09-21 21:03:01.000000000 -0300
> > +++ linux-3.0.4/drivers/base/firmware_class.c	2011-09-21 21:03:13.000000000 -0300
> > @@ -523,6 +523,7 @@ static int _request_firmware(const struc
> >  
> >  	if (WARN_ON(usermodehelper_is_disabled())) {
> >  		dev_err(device, "firmware: %s will not be loaded\n", name);
> > +		*firmware_p = NULL;
> >  		return -EBUSY;
> >  	}
> 
> Looks like f45f3c1f3f616 needs backporting to stable, if it hasn't
> happened yet.

What stable tree?  That patch was in the 2.6.36 release, so 3.0-stable
doesn't need it, right?

> Oh, and then there's caca9510ff4e5 too which adds this
> exit path to the goto out label as the rest of the function.

But that was only due to other problems.

Again, I don't understand what the issue really is here.

confused,

greg k-h

Re: [PATCH] Initialize pointer on request_firmware

[Borislav Petkov]

On Thu, Sep 22, 2011 at 10:18:29AM -0700, Greg KH wrote:
> On Thu, Sep 22, 2011 at 11:23:47AM +0200, Borislav Petkov wrote:
> > On Wed, Sep 21, 2011 at 11:55:15PM -0300, Lucas C. Villa Real wrote:
> > > Hello, folks,
> > > 
> > > I've seen some kernel oopses when suspending my machine. The problem comes from isight_firmware, which assumes that, on error, a call to request_firmware() will initialize the provided pointer to the firmware image to NULL.
> > > 
> > > The patch below fixes the isight_firmware side of the problem and also ensures that request_firmware() always sets the pointer to NULL on such cases (it currently does that for all except one situation).
> > > 
> > > Signed-off-by: Lucas C. Villa Real <lucasvr@gobolinux.org>
> > > 
> > > --- linux-3.0.4/drivers/base/firmware_class.c.orig	2011-09-21 21:03:01.000000000 -0300
> > > +++ linux-3.0.4/drivers/base/firmware_class.c	2011-09-21 21:03:13.000000000 -0300
> > > @@ -523,6 +523,7 @@ static int _request_firmware(const struc
> > >  
> > >  	if (WARN_ON(usermodehelper_is_disabled())) {
> > >  		dev_err(device, "firmware: %s will not be loaded\n", name);
> > > +		*firmware_p = NULL;
> > >  		return -EBUSY;
> > >  	}
> > 
> > Looks like f45f3c1f3f616 needs backporting to stable, if it hasn't
> > happened yet.
> 
> What stable tree?  That patch was in the 2.6.36 release, so 3.0-stable
> doesn't need it, right?

Judging by the diff lines above, 3.0.4 doesn't seem to have it. Wait,
lemme check... uh no, I can't, the damn k.org thing is still down :-(.

> > Oh, and then there's caca9510ff4e5 too which adds this
> > exit path to the goto out label as the rest of the function.
> 
> But that was only due to other problems.

That second one adds the goto thing to the "out:" label where the
firmware_p gets NULLed. IOW, it prepares the code for f45f3c1f3f616.

Does that make more sense now?

-- 
Regards/Gruss,
Boris.

Re: [PATCH] Initialize pointer on request_firmware

[Greg KH]

On Thu, Sep 22, 2011 at 07:54:06PM +0200, Borislav Petkov wrote:
> On Thu, Sep 22, 2011 at 10:18:29AM -0700, Greg KH wrote:
> > On Thu, Sep 22, 2011 at 11:23:47AM +0200, Borislav Petkov wrote:
> > > On Wed, Sep 21, 2011 at 11:55:15PM -0300, Lucas C. Villa Real wrote:
> > > > Hello, folks,
> > > > 
> > > > I've seen some kernel oopses when suspending my machine. The problem comes from isight_firmware, which assumes that, on error, a call to request_firmware() will initialize the provided pointer to the firmware image to NULL.
> > > > 
> > > > The patch below fixes the isight_firmware side of the problem and also ensures that request_firmware() always sets the pointer to NULL on such cases (it currently does that for all except one situation).
> > > > 
> > > > Signed-off-by: Lucas C. Villa Real <lucasvr@gobolinux.org>
> > > > 
> > > > --- linux-3.0.4/drivers/base/firmware_class.c.orig	2011-09-21 21:03:01.000000000 -0300
> > > > +++ linux-3.0.4/drivers/base/firmware_class.c	2011-09-21 21:03:13.000000000 -0300
> > > > @@ -523,6 +523,7 @@ static int _request_firmware(const struc
> > > >  
> > > >  	if (WARN_ON(usermodehelper_is_disabled())) {
> > > >  		dev_err(device, "firmware: %s will not be loaded\n", name);
> > > > +		*firmware_p = NULL;
> > > >  		return -EBUSY;
> > > >  	}
> > > 
> > > Looks like f45f3c1f3f616 needs backporting to stable, if it hasn't
> > > happened yet.
> > 
> > What stable tree?  That patch was in the 2.6.36 release, so 3.0-stable
> > doesn't need it, right?
> 
> Judging by the diff lines above, 3.0.4 doesn't seem to have it. Wait,
> lemme check... uh no, I can't, the damn k.org thing is still down :-(.

The patch was in the 2.6.36 kernel, so by nature of that, 3.0 will also
have it, right?

> > > Oh, and then there's caca9510ff4e5 too which adds this
> > > exit path to the goto out label as the rest of the function.
> > 
> > But that was only due to other problems.
> 
> That second one adds the goto thing to the "out:" label where the
> firmware_p gets NULLed. IOW, it prepares the code for f45f3c1f3f616.
> 
> Does that make more sense now?

Nope, sorry, still confused :)

How about, what patches are needed for the 3.0-stable kernel tree in
order to resolve these issue?  Git commit ids please.

thanks,

greg k-h

Re: [PATCH] Initialize pointer on request_firmware

[Borislav Petkov]

On Thu, Sep 22, 2011 at 11:03:27AM -0700, Greg KH wrote:
> The patch was in the 2.6.36 kernel, so by nature of that, 3.0 will also
> have it, right?

Ok, a colleague of mine gave me 3.0.4 to stare at - and you're right
the patch is there. So what is still missing is caca9510ff4e5 from
upstream which reroutes the exit path so that firmware_p is NULLed
before returning -EBUSY.

Lucas, can you apply caca9510ff4e5 from upstream and verify it fixes the
issue for ya?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH] Initialize pointer on request_firmware

[Lucas C. Villa Real]

On Thu, Sep 22, 2011 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Thu, Sep 22, 2011 at 11:03:27AM -0700, Greg KH wrote:
>> The patch was in the 2.6.36 kernel, so by nature of that, 3.0 will also
>> have it, right?
>
> Ok, a colleague of mine gave me 3.0.4 to stare at - and you're right
> the patch is there. So what is still missing is caca9510ff4e5 from
> upstream which reroutes the exit path so that firmware_p is NULLed
> before returning -EBUSY.
>
> Lucas, can you apply caca9510ff4e5 from upstream and verify it fixes the
> issue for ya?

Sure. I'll do that tonight and will let you now -- the laptop I'm
using right now doesn't suffer from that problem.

Thanks,

-- 
Lucas
"If you're looking for a reason I've a reason to give: pleasure,
little treasure"

Re: [PATCH] Initialize pointer on request_firmware

[Lucas C. Villa Real]

On Thu, Sep 22, 2011 at 4:05 PM, Lucas C. Villa Real
<lucasvr@gobolinux.org> wrote:
> On Thu, Sep 22, 2011 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote:
>> On Thu, Sep 22, 2011 at 11:03:27AM -0700, Greg KH wrote:
>>> The patch was in the 2.6.36 kernel, so by nature of that, 3.0 will also
>>> have it, right?
>>
>> Ok, a colleague of mine gave me 3.0.4 to stare at - and you're right
>> the patch is there. So what is still missing is caca9510ff4e5 from
>> upstream which reroutes the exit path so that firmware_p is NULLed
>> before returning -EBUSY.
>>
>> Lucas, can you apply caca9510ff4e5 from upstream and verify it fixes the
>> issue for ya?
>
> Sure. I'll do that tonight and will let you now -- the laptop I'm
> using right now doesn't suffer from that problem.

caca9510ff4e5 fixes it. Thanks for the pointer!

Best regards,

-- 
Lucas
"If you're looking for a reason I've a reason to give: pleasure,
little treasure"

Re: [PATCH] Initialize pointer on request_firmware

[Borislav Petkov]

On Fri, Sep 23, 2011 at 12:51:36PM -0300, Lucas C. Villa Real wrote:
> On Thu, Sep 22, 2011 at 4:05 PM, Lucas C. Villa Real
> <lucasvr@gobolinux.org> wrote:
> > On Thu, Sep 22, 2011 at 3:20 PM, Borislav Petkov <bp@alien8.de> wrote:
> >> On Thu, Sep 22, 2011 at 11:03:27AM -0700, Greg KH wrote:
> >>> The patch was in the 2.6.36 kernel, so by nature of that, 3.0 will also
> >>> have it, right?
> >>
> >> Ok, a colleague of mine gave me 3.0.4 to stare at - and you're right
> >> the patch is there. So what is still missing is caca9510ff4e5 from
> >> upstream which reroutes the exit path so that firmware_p is NULLed
> >> before returning -EBUSY.
> >>
> >> Lucas, can you apply caca9510ff4e5 from upstream and verify it fixes the
> >> issue for ya?
> >
> > Sure. I'll do that tonight and will let you now -- the laptop I'm
> > using right now doesn't suffer from that problem.
> 
> caca9510ff4e5 fixes it. Thanks for the pointer!

Looks like the ball is with Greg now :-).

Thanks.

-- 
Regards/Gruss,
    Boris.