All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers
@ 2013-08-09 18:23 Philippe De Swert
  2013-08-12 18:08 ` Felipe Balbi
  0 siblings, 1 reply; 4+ messages in thread
From: Philippe De Swert @ 2013-08-09 18:23 UTC (permalink / raw)
  To: linux-usb, gregkh, balbi, linux-kernel; +Cc: Philippe De Swert

Some bad gadget drivers do not check the return status of usb_add_config.
Thus they get a not correctly initialized config and when this gadget gets
deactivated the whole kernel crashes. Since on initialization failure cdev
is set to NULL it can be used to detect this problem situation. It can be
argued that the faulty gadget driver should be fixed, but imho it is better
to avoid crashing the kernel and letting the gadget developer know he/she
is making a mistake. And have the developer of said gadget driver then fix
the problem of course.

Signed-off-by: Philippe De Swert <philippe.deswert@jollamobile.com>
---
 drivers/usb/gadget/composite.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 55f4df6..e019bb5 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -848,12 +848,18 @@ void usb_remove_config(struct usb_composite_dev *cdev,
 {
 	unsigned long flags;
 
+	if (config->cdev == NULL) {
+		pr_warn("Calling usb_remove_config without a matching usb_add_config!\n");
+		goto end;
+	}
+
 	spin_lock_irqsave(&cdev->lock, flags);
 
 	if (cdev->config == config)
 		reset_config(cdev);
 
 	spin_unlock_irqrestore(&cdev->lock, flags);
+end:
 
 	remove_config(cdev, config);
 }
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers
  2013-08-09 18:23 [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers Philippe De Swert
@ 2013-08-12 18:08 ` Felipe Balbi
  2013-08-14 22:40   ` Philippe De Swert
  0 siblings, 1 reply; 4+ messages in thread
From: Felipe Balbi @ 2013-08-12 18:08 UTC (permalink / raw)
  To: Philippe De Swert; +Cc: linux-usb, gregkh, balbi, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Fri, Aug 09, 2013 at 09:23:08PM +0300, Philippe De Swert wrote:
> Some bad gadget drivers do not check the return status of usb_add_config.

fix the gadget driver

> Thus they get a not correctly initialized config and when this gadget gets
> deactivated the whole kernel crashes. Since on initialization failure cdev
> is set to NULL it can be used to detect this problem situation. It can be
> argued that the faulty gadget driver should be fixed, but imho it is better
> to avoid crashing the kernel and letting the gadget developer know he/she
> is making a mistake. And have the developer of said gadget driver then fix
> the problem of course.
> 
> Signed-off-by: Philippe De Swert <philippe.deswert@jollamobile.com>
> ---
>  drivers/usb/gadget/composite.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 55f4df6..e019bb5 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -848,12 +848,18 @@ void usb_remove_config(struct usb_composite_dev *cdev,
>  {
>  	unsigned long flags;
>  
> +	if (config->cdev == NULL) {
> +		pr_warn("Calling usb_remove_config without a matching usb_add_config!\n");
> +		goto end;
> +	}

I would take a WARN() only, but let the crash happen and fix the gadget
driver.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers
  2013-08-12 18:08 ` Felipe Balbi
@ 2013-08-14 22:40   ` Philippe De Swert
  2013-08-27 18:55     ` Felipe Balbi
  0 siblings, 1 reply; 4+ messages in thread
From: Philippe De Swert @ 2013-08-14 22:40 UTC (permalink / raw)
  To: balbi; +Cc: linux-usb, gregkh, linux-kernel

Hi,

On 12/08/13 21:08, Felipe Balbi wrote:
> On Fri, Aug 09, 2013 at 09:23:08PM +0300, Philippe De Swert wrote:
>> Some bad gadget drivers do not check the return status of usb_add_config.
>
> fix the gadget driver

As stated in my comment (see below) that is indeed what should happen. 
But we cannot fix it in future new gadget drivers, is it thus not better
to avoid a crash? The gadget driver will not work as expected anyway 
when this occurs and the print will at least give an indication why.

>> Thus they get a not correctly initialized config and when this gadget gets
>> deactivated the whole kernel crashes. Since on initialization failure cdev
>> is set to NULL it can be used to detect this problem situation. It can be
>> argued that the faulty gadget driver should be fixed, but imho it is better
>> to avoid crashing the kernel and letting the gadget developer know he/she
>> is making a mistake. And have the developer of said gadget driver then fix
>> the problem of course.
>>
>> Signed-off-by: Philippe De Swert <philippe.deswert@jollamobile.com>
>> ---
>>   drivers/usb/gadget/composite.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 55f4df6..e019bb5 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -848,12 +848,18 @@ void usb_remove_config(struct usb_composite_dev *cdev,
>>   {
>>   	unsigned long flags;
>>
>> +	if (config->cdev == NULL) {
>> +		pr_warn("Calling usb_remove_config without a matching usb_add_config!\n");
>> +		goto end;
>> +	}
>
> I would take a WARN() only, but let the crash happen and fix the gadget
> driver.
>

Since the kernel will crash with a NULL dereference anyway is BUG_ON not 
a better approach then?

Regards,

Philippe

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers
  2013-08-14 22:40   ` Philippe De Swert
@ 2013-08-27 18:55     ` Felipe Balbi
  0 siblings, 0 replies; 4+ messages in thread
From: Felipe Balbi @ 2013-08-27 18:55 UTC (permalink / raw)
  To: Philippe De Swert; +Cc: balbi, linux-usb, gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Thu, Aug 15, 2013 at 01:40:01AM +0300, Philippe De Swert wrote:
> Hi,
> 
> On 12/08/13 21:08, Felipe Balbi wrote:
> >On Fri, Aug 09, 2013 at 09:23:08PM +0300, Philippe De Swert wrote:
> >>Some bad gadget drivers do not check the return status of usb_add_config.
> >
> >fix the gadget driver
> 
> As stated in my comment (see below) that is indeed what should
> happen. But we cannot fix it in future new gadget drivers, is it thus
> not better
> to avoid a crash? The gadget driver will not work as expected anyway
> when this occurs and the print will at least give an indication why.

if we avoid a crash, we will be decreasing the urgency of fixing the
gadget driver bug.

Let's leave it as is. I can't live thinking that maybe in the future
there will be gadget drivers doing different things...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-08-27 18:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-09 18:23 [PATCH] usb: gadget/composite : Avoid crash with bad gadget drivers Philippe De Swert
2013-08-12 18:08 ` Felipe Balbi
2013-08-14 22:40   ` Philippe De Swert
2013-08-27 18:55     ` Felipe Balbi

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.