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 <[email protected]>
Reported-by: [email protected]
---
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
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 <[email protected]>
> Reported-by: [email protected]
> ---
> 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
> 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
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
On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern <[email protected]> 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.
On Tue, 25 Sep 2018, Andrey Konovalov wrote:
> On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern <[email protected]> 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
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
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
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" <[email protected]>
> To: "Vladis Dronov" <[email protected]>
> Cc: "Andrey Konovalov" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "Oliver Neukum"
> <[email protected]>, "Hans de Goede" <[email protected]>, "syzkaller" <[email protected]>, "USB list"
> <[email protected]>, "LKML" <[email protected]>, "stable" <[email protected]>
> 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.
>