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
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
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 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.
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
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" 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 beingcalled, 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.
linux-stable-mirror@lists.linaro.org
-
Alan Stern -
Andrey Konovalov -
Vladis Dronov