[PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Vladis Dronov]

ps->dev->actconfig can be NULL and cause NULL-deref in usb_find_alt_setting()
before c9a4cb204e9e. fix this anyway by checking that ps->dev->actconfig is not
NULL, so usb_find_alt_setting() is not called with a known-bad argument.

Signed-off-by: Vladis Dronov <vdronov@redhat.com>
Reported-by: syzbot+19c3aaef85a89d451eac@syzkaller.appspotmail.com
---
 drivers/usb/core/devio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6ce77b33da61..26047620b003 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -824,7 +824,7 @@ static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
 	 * class specification, which we always want to allow as it is used
 	 * to query things like ink level, etc.
 	 */
-	if (requesttype == 0xa1 && request == 0) {
+	if (requesttype == 0xa1 && request == 0 && ps->dev->actconfig) {
 		alt_setting = usb_find_alt_setting(ps->dev->actconfig,
 						   index >> 8, index & 0xff);
 		if (alt_setting
-- 
2.14.4

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Alan Stern]

On Tue, 25 Sep 2018, Vladis Dronov wrote:

> ps->dev->actconfig can be NULL and cause NULL-deref in usb_find_alt_setting()
> before c9a4cb204e9e. fix this anyway by checking that ps->dev->actconfig is not
> NULL, so usb_find_alt_setting() is not called with a known-bad argument.

What reason is there for having two different fixes for the same bug?  
This one isn't going to get into any mainline trees that don't already 
have c9a4cb204e9e.

Alan Stern

> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> Reported-by: syzbot+19c3aaef85a89d451eac@syzkaller.appspotmail.com
> ---
>  drivers/usb/core/devio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 6ce77b33da61..26047620b003 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -824,7 +824,7 @@ static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  	 * class specification, which we always want to allow as it is used
>  	 * to query things like ink level, etc.
>  	 */
> -	if (requesttype == 0xa1 && request == 0) {
> +	if (requesttype == 0xa1 && request == 0 && ps->dev->actconfig) {
>  		alt_setting = usb_find_alt_setting(ps->dev->actconfig,
>  						   index >> 8, index & 0xff);
>  		if (alt_setting

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Vladis Dronov]

> What reason is there for having two different fixes for the same bug?
> This one isn't going to get into any mainline trees that don't already
> have c9a4cb204e9e.

I believe this is the right thing to do, so usb_find_alt_setting()
is not called with a known-bad argument.

Honestly, I would change "if (!config)" in usb_find_alt_setting() to
"BUG_ON(!config)" so we know when its callers do smth wrong and go
fix callers. Unfortunately, I understand this hardly will be accepted.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Alan Stern]

On Tue, 25 Sep 2018, Vladis Dronov wrote:

> > What reason is there for having two different fixes for the same bug?
> > This one isn't going to get into any mainline trees that don't already
> > have c9a4cb204e9e.
> 
> I believe this is the right thing to do, so usb_find_alt_setting()
> is not called with a known-bad argument.
> 
> Honestly, I would change "if (!config)" in usb_find_alt_setting() to
> "BUG_ON(!config)" so we know when its callers do smth wrong and go

(You'll be lucky if Linus doesn't see that.  He yells at anybody who
suggests adding BUG_ON for anything that doesn't completely crash the
kernel.  The basic problem is that "BUG_ON" is not a good name: That
routine doesn't really report bugs; instead it brings everything to a
halt in situations where the kernel is unable to proceed.  In practice 
this tends to make actual debugging more difficult.)

> fix callers. Unfortunately, I understand this hardly will be accepted.

How is this different from calling kfree() with a NULL argument?

Alan Stern

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Andrey Konovalov]

On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 25 Sep 2018, Vladis Dronov wrote:
>
>> > What reason is there for having two different fixes for the same bug?
>> > This one isn't going to get into any mainline trees that don't already
>> > have c9a4cb204e9e.
>>
>> I believe this is the right thing to do, so usb_find_alt_setting()
>> is not called with a known-bad argument.
>>
>> Honestly, I would change "if (!config)" in usb_find_alt_setting() to
>> "BUG_ON(!config)" so we know when its callers do smth wrong and go
>
> (You'll be lucky if Linus doesn't see that.  He yells at anybody who
> suggests adding BUG_ON for anything that doesn't completely crash the
> kernel.  The basic problem is that "BUG_ON" is not a good name: That
> routine doesn't really report bugs; instead it brings everything to a
> halt in situations where the kernel is unable to proceed.  In practice
> this tends to make actual debugging more difficult.)

What about adding a WARN_ON()? It doesn't crash the kernel and it will
be detected and reported by syzbot.

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Alan Stern]

On Tue, 25 Sep 2018, Andrey Konovalov wrote:

> On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 25 Sep 2018, Vladis Dronov wrote:
> >
> >> > What reason is there for having two different fixes for the same bug?
> >> > This one isn't going to get into any mainline trees that don't already
> >> > have c9a4cb204e9e.
> >>
> >> I believe this is the right thing to do, so usb_find_alt_setting()
> >> is not called with a known-bad argument.
> >>
> >> Honestly, I would change "if (!config)" in usb_find_alt_setting() to
> >> "BUG_ON(!config)" so we know when its callers do smth wrong and go
> >
> > (You'll be lucky if Linus doesn't see that.  He yells at anybody who
> > suggests adding BUG_ON for anything that doesn't completely crash the
> > kernel.  The basic problem is that "BUG_ON" is not a good name: That
> > routine doesn't really report bugs; instead it brings everything to a
> > halt in situations where the kernel is unable to proceed.  In practice
> > this tends to make actual debugging more difficult.)
> 
> What about adding a WARN_ON()? It doesn't crash the kernel and it will
> be detected and reported by syzbot.

Sure, we could do that.  But would be the point?  After c9a4cb204e9e, 
calling usb_find_alt_setting() with a NULL config is no more of a bug 
than calling kfree() with a NULL pointer.  You wouldn't want to put a 
WARN_ON in kfree(), would you?

Alan Stern

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Vladis Dronov]

Hello, Alan, Andrey, all,

> > > (You'll be lucky if Linus doesn't see that.  He yells at anybody who
> > > suggests adding BUG_ON for anything that doesn't completely crash

Now, may be not )

> > > How is this different from calling kfree() with a NULL argument?

It is not, it is the same case.

> > What about adding a WARN_ON()? It doesn't crash the kernel and it will
> > be detected and reported by syzbot.

Yes, that would be a great solution.

> Sure, we could do that.  But would be the point?

We know when usb_find_alt_setting() callers do smth weird and go fix them.

> After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is
> no more of a bug than calling kfree() with a NULL pointer.

Yes, exactly.

> You wouldn't want to put a WARN_ON in kfree(), would you?

Honestly, in the ideal world I would, again, to be aware when some code does
something weird so we know about it. But this world is this world, it needs
more performance to the throne of performance.

I have no other arguments except the above, please, feel free to not to accept
my patch.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Alan Stern]

On Tue, 25 Sep 2018, Vladis Dronov wrote:

> > > What about adding a WARN_ON()? It doesn't crash the kernel and it will
> > > be detected and reported by syzbot.
> 
> Yes, that would be a great solution.
> 
> > Sure, we could do that.  But would be the point?
> 
> We know when usb_find_alt_setting() callers do smth weird and go fix them.
> 
> > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is
> > no more of a bug than calling kfree() with a NULL pointer.
> 
> Yes, exactly.
> 
> > You wouldn't want to put a WARN_ON in kfree(), would you?
> 
> Honestly, in the ideal world I would, again, to be aware when some code does
> something weird so we know about it. But this world is this world, it needs
> more performance to the throne of performance.

But is it really worthwhile?  In terms of catching bugs, this would
help in only one situation: when the programmer thinks the argument
should always be non-NULL because a NULL argument indicates a bug.  
Such situations seem to be relatively rare, and we can handle them by
inserting a WARN_ON() at the call site if need be.

So it's a choice between:

     1. Putting a single test for NULL in the function being called, 
	together with WARN_ON() at a small number of call sites, or

     2. Putting a WARN_ON() (or allowing a crash) in the function being
	called, together with tests for NULL at a potentially large 
	number of call sites.

1 has two advantages over 2.  First, it involves adding less code 
overall.  Second, it doesn't require the programmer to remember to add 
special code (a test or a WARN_ON) in situation where it doesn't 
matter -- presumably the majority of them.

Now consider the case at hand: the call to usb_find_alt_setting() from
check_ctrlrecip().  In this case ps->dev->actconfig being NULL doesn't
indicate an error or a bug; it merely indicates that the user is trying
to send a control request to a device which happens to be unconfigured,
which is a perfectly valid thing to do.  Therefore it shouldn't require 
any special handling at the call site.

Alan Stern

Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

[Vladis Dronov]

Hello, Alan,

> Now consider the case at hand: the call to usb_find_alt_setting() from
> check_ctrlrecip().  In this case ps->dev->actconfig being NULL doesn't
> indicate an error or a bug; it merely indicates that the user is trying
> to send a control request to a device which happens to be unconfigured,
> which is a perfectly valid thing to do.  Therefore it shouldn't require
> any special handling at the call site.
> 
> Alan Stern

Thank you for the explanation and a detailed response.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

----- Original Message -----
> From: "Alan Stern" <stern@rowland.harvard.edu>
> To: "Vladis Dronov" <vdronov@redhat.com>
> Cc: "Andrey Konovalov" <andreyknvl@google.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Oliver Neukum"
> <oneukum@suse.com>, "Hans de Goede" <hdegoede@redhat.com>, "syzkaller" <syzkaller@googlegroups.com>, "USB list"
> <linux-usb@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, "stable" <stable@vger.kernel.org>
> Sent: Tuesday, September 25, 2018 10:44:14 PM
> Subject: Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
> 
> On Tue, 25 Sep 2018, Vladis Dronov wrote:
> 
> > > > What about adding a WARN_ON()? It doesn't crash the kernel and it will
> > > > be detected and reported by syzbot.
> > 
> > Yes, that would be a great solution.
> > 
> > > Sure, we could do that.  But would be the point?
> > 
> > We know when usb_find_alt_setting() callers do smth weird and go fix them.
> > 
> > > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is
> > > no more of a bug than calling kfree() with a NULL pointer.
> > 
> > Yes, exactly.
> > 
> > > You wouldn't want to put a WARN_ON in kfree(), would you?
> > 
> > Honestly, in the ideal world I would, again, to be aware when some code
> > does
> > something weird so we know about it. But this world is this world, it needs
> > more performance to the throne of performance.
> 
> But is it really worthwhile?  In terms of catching bugs, this would
> help in only one situation: when the programmer thinks the argument
> should always be non-NULL because a NULL argument indicates a bug.
> Such situations seem to be relatively rare, and we can handle them by
> inserting a WARN_ON() at the call site if need be.
> 
> So it's a choice between:
> 
>      1. Putting a single test for NULL in the function being called,
> 	together with WARN_ON() at a small number of call sites, or
> 
>      2. Putting a WARN_ON() (or allowing a crash) in the function being
> 	called, together with tests for NULL at a potentially large
> 	number of call sites.
> 
> 1 has two advantages over 2.  First, it involves adding less code
> overall.  Second, it doesn't require the programmer to remember to add
> special code (a test or a WARN_ON) in situation where it doesn't
> matter -- presumably the majority of them.
>