* [PATCH] USB: core: Fix a NULL pointer dereference
@ 2023-09-08 15:39 Yuran Pereira
2023-09-08 16:42 ` Alan Stern
0 siblings, 1 reply; 5+ messages in thread
From: Yuran Pereira @ 2023-09-08 15:39 UTC (permalink / raw)
To: gregkh
Cc: Yuran Pereira, stern, royluo, christophe.jaillet, raychi,
linux-usb, linux-kernel, syzbot+c063a4e176681d2e0380
When the call to dev_set_name() in the usb_hub_create_port_device
function fails to set the device's kobject's name field,
the subsequent call to device_register() is bound to fail and cause
a NULL pointer derefference, because kobject_add(), which is in the
call sequence, expects the name field to be set before it is called
This patch adds code to perform error checking for dev_set_name()'s
return value. If the call to dev_set_name() was unsuccessful,
usb_hub_create_port_device() returns with an error.
PS: The patch also frees port_dev->req and port_dev before returning.
However, I am not sure if that is necessary, because port_dev->req
and port_dev are not freed when device_register() fails. Would be
happy if someone could help me understand why that is, and whether I
should keep those kfree calls in my patch.
dashboard link: https://syzkaller.appspot.com/bug?extid=c063a4e176681d2e0380
Reported-by: syzbot+c063a4e176681d2e0380@syzkaller.appspotmail.com
Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
drivers/usb/core/port.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 77be0dc28da9..e546e92e31a7 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -707,8 +707,14 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
port_dev->dev.driver = &usb_port_driver;
if (hub_is_superspeed(hub->hdev))
port_dev->is_superspeed = 1;
- dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
- port1);
+
+ retval = dev_set_name(&port_dev->dev, "%s-port%d",
+ dev_name(&hub->hdev->dev), port1);
+ if (retval < 0) {
+ kfree(port_dev->req);
+ kfree(port_dev);
+ return retval;
+ }
mutex_init(&port_dev->status_lock);
retval = device_register(&port_dev->dev);
if (retval) {
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: core: Fix a NULL pointer dereference
2023-09-08 15:39 [PATCH] USB: core: Fix a NULL pointer dereference Yuran Pereira
@ 2023-09-08 16:42 ` Alan Stern
[not found] ` <AM9P192MB12670D185D208AFA51B8348EE8ECA@AM9P192MB1267.EURP192.PROD.OUTLOOK.COM>
0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2023-09-08 16:42 UTC (permalink / raw)
To: Yuran Pereira
Cc: gregkh, royluo, christophe.jaillet, raychi, linux-usb,
linux-kernel, syzbot+c063a4e176681d2e0380
On Fri, Sep 08, 2023 at 09:09:37PM +0530, Yuran Pereira wrote:
>
> When the call to dev_set_name() in the usb_hub_create_port_device
> function fails to set the device's kobject's name field,
> the subsequent call to device_register() is bound to fail and cause
> a NULL pointer derefference, because kobject_add(), which is in the
> call sequence, expects the name field to be set before it is called
>
>
> This patch adds code to perform error checking for dev_set_name()'s
> return value. If the call to dev_set_name() was unsuccessful,
> usb_hub_create_port_device() returns with an error.
>
>
> PS: The patch also frees port_dev->req and port_dev before returning.
> However, I am not sure if that is necessary, because port_dev->req
> and port_dev are not freed when device_register() fails. Would be
> happy if someone could help me understand why that is, and whether I
> should keep those kfree calls in my patch.
>
> dashboard link: https://syzkaller.appspot.com/bug?extid=c063a4e176681d2e0380
>
> Reported-by: syzbot+c063a4e176681d2e0380@syzkaller.appspotmail.com
It shouldn't be necessary to check the return value from dev_set_name().
Most of its other call sites ignore the return value. In fact, only one
of the call sites in drivers/base/core.c does check the return value!
Furthermore, device_add() includes the following test for whether the
device's name has been set:
if (!dev_name(dev)) {
error = -EINVAL;
goto name_error;
}
The real question is why this test did not prevent the bug from
occurring. Until you can answer that question, any fix you propose is
highly questionable.
Alan Stern
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: core: Fix a NULL pointer dereference
[not found] ` <AM9P192MB12670D185D208AFA51B8348EE8ECA@AM9P192MB1267.EURP192.PROD.OUTLOOK.COM>
@ 2023-09-09 14:36 ` Alan Stern
2023-09-09 15:35 ` gregkh
[not found] ` <AS8P192MB1269A9732001D142F3272ACDE8F6A@AS8P192MB1269.EURP192.PROD.OUTLOOK.COM>
0 siblings, 2 replies; 5+ messages in thread
From: Alan Stern @ 2023-09-09 14:36 UTC (permalink / raw)
To: Yuran Pereira, Andy Shevchenko
Cc: gregkh, royluo, christophe.jaillet, raychi, linux-usb,
linux-kernel, syzbot+c063a4e176681d2e0380
On Sat, Sep 09, 2023 at 06:28:12AM +0000, Yuran Pereira wrote:
> Hello Alan,
>
> Thank you for elucidating that.
>
> So, this bug is present on the mainline tree which is where syzkaller
> found it. My patch was also based on the mainline tree.
>
> I just ran the same reproducer against a kernel compiled from the usb
> tree, and, as you suggested, the test you mentioned does in fact,
> prevent the bug from occurring.
>
> Please forgive my ignorance; I am a new contributor to the community.
> But in this situation how should I proceed? Is there even a need to
> submit a patch, or will the code currently present in the usb tree
> eventually be reflected in the mainline?
The first step is to find the difference between the mainline and USB
trees that is responsible for this change in behavior. A quick check of
the Git logs shows that the change was caused by commit d21fdd07cea4
("driver core: Return proper error code when dev_set_name() fails"),
written by Andy Shevchenko. As a result of this commit, the code in
device_add() now says:
if (dev_name(dev))
error = 0;
/* subsystems can specify simple device enumeration */
else if (dev->bus && dev->bus->dev_name)
error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
if (error)
goto name_error;
This obviously omits a final "else" clause; it should say:
if (dev_name(dev))
error = 0;
/* subsystems can specify simple device enumeration */
else if (dev->bus && dev->bus->dev_name)
error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
+ else
+ error = -EINVAL;
if (error)
goto name_error;
So to answer your questions: No, the code in the USB tree will not find
its way into mainline. The opposite will happen: The mainline code will
land in the USB tree. Which means that yes, there is a need to submit a
patch. You can go ahead and write this up for submission, or I can
submit it for you. Or you can check with Andy and see if he wants to
fix the problem in a different way.
Alan Stern
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: core: Fix a NULL pointer dereference
2023-09-09 14:36 ` Alan Stern
@ 2023-09-09 15:35 ` gregkh
[not found] ` <AS8P192MB1269A9732001D142F3272ACDE8F6A@AS8P192MB1269.EURP192.PROD.OUTLOOK.COM>
1 sibling, 0 replies; 5+ messages in thread
From: gregkh @ 2023-09-09 15:35 UTC (permalink / raw)
To: Alan Stern
Cc: Yuran Pereira, Andy Shevchenko, royluo, christophe.jaillet,
raychi, linux-usb, linux-kernel, syzbot+c063a4e176681d2e0380
On Sat, Sep 09, 2023 at 10:36:53AM -0400, Alan Stern wrote:
> On Sat, Sep 09, 2023 at 06:28:12AM +0000, Yuran Pereira wrote:
> > Hello Alan,
> >
> > Thank you for elucidating that.
> >
> > So, this bug is present on the mainline tree which is where syzkaller
> > found it. My patch was also based on the mainline tree.
> >
> > I just ran the same reproducer against a kernel compiled from the usb
> > tree, and, as you suggested, the test you mentioned does in fact,
> > prevent the bug from occurring.
> >
> > Please forgive my ignorance; I am a new contributor to the community.
> > But in this situation how should I proceed? Is there even a need to
> > submit a patch, or will the code currently present in the usb tree
> > eventually be reflected in the mainline?
>
> The first step is to find the difference between the mainline and USB
> trees that is responsible for this change in behavior. A quick check of
> the Git logs shows that the change was caused by commit d21fdd07cea4
> ("driver core: Return proper error code when dev_set_name() fails"),
> written by Andy Shevchenko. As a result of this commit, the code in
> device_add() now says:
>
> if (dev_name(dev))
> error = 0;
> /* subsystems can specify simple device enumeration */
> else if (dev->bus && dev->bus->dev_name)
> error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
> if (error)
> goto name_error;
>
> This obviously omits a final "else" clause; it should say:
>
> if (dev_name(dev))
> error = 0;
> /* subsystems can specify simple device enumeration */
> else if (dev->bus && dev->bus->dev_name)
> error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
> + else
> + error = -EINVAL;
> if (error)
> goto name_error;
And:
https://lore.kernel.org/r/20230828145824.3895288-1-andriy.shevchenko@linux.intel.com
is the fix for this which will show up in time for 6.6-final.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: core: Fix a NULL pointer dereference
[not found] ` <AS8P192MB1269A9732001D142F3272ACDE8F6A@AS8P192MB1269.EURP192.PROD.OUTLOOK.COM>
@ 2023-09-15 1:42 ` Alan Stern
0 siblings, 0 replies; 5+ messages in thread
From: Alan Stern @ 2023-09-15 1:42 UTC (permalink / raw)
To: Yuran Pereira
Cc: Andy Shevchenko, gregkh, royluo, christophe.jaillet, raychi,
linux-usb, linux-kernel, syzbot+c063a4e176681d2e0380
On Fri, Sep 15, 2023 at 12:57:58AM +0000, Yuran Pereira wrote:
> Hello Alan,
>
> Thank you for the detailed explanation.
>
> Apologies for the delay replying.
> Please, feel free to submit the patch.
No need; Andy Shevchenko already submitted the same patch some time ago
and it has been merged.
Alan Stern
> ________________________________
> De: Alan Stern <stern@rowland.harvard.edu>
> Enviado: 9 de setembro de 2023 14:36
> Para: Yuran Pereira <yuran.pereira@hotmail.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; royluo@google.com <royluo@google.com>; christophe.jaillet@wanadoo.fr <christophe.jaillet@wanadoo.fr>; raychi@google.com <raychi@google.com>; linux-usb@vger.kernel.org <linux-usb@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; syzbot+c063a4e176681d2e0380@syzkaller.appspotmail.com <syzbot+c063a4e176681d2e0380@syzkaller.appspotmail.com>
> Assunto: Re: [PATCH] USB: core: Fix a NULL pointer dereference
>
> On Sat, Sep 09, 2023 at 06:28:12AM +0000, Yuran Pereira wrote:
> > Hello Alan,
> >
> > Thank you for elucidating that.
> >
> > So, this bug is present on the mainline tree which is where syzkaller
> > found it. My patch was also based on the mainline tree.
> >
> > I just ran the same reproducer against a kernel compiled from the usb
> > tree, and, as you suggested, the test you mentioned does in fact,
> > prevent the bug from occurring.
> >
> > Please forgive my ignorance; I am a new contributor to the community.
> > But in this situation how should I proceed? Is there even a need to
> > submit a patch, or will the code currently present in the usb tree
> > eventually be reflected in the mainline?
>
> The first step is to find the difference between the mainline and USB
> trees that is responsible for this change in behavior. A quick check of
> the Git logs shows that the change was caused by commit d21fdd07cea4
> ("driver core: Return proper error code when dev_set_name() fails"),
> written by Andy Shevchenko. As a result of this commit, the code in
> device_add() now says:
>
> if (dev_name(dev))
> error = 0;
> /* subsystems can specify simple device enumeration */
> else if (dev->bus && dev->bus->dev_name)
> error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
> if (error)
> goto name_error;
>
> This obviously omits a final "else" clause; it should say:
>
> if (dev_name(dev))
> error = 0;
> /* subsystems can specify simple device enumeration */
> else if (dev->bus && dev->bus->dev_name)
> error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
> + else
> + error = -EINVAL;
> if (error)
> goto name_error;
>
> So to answer your questions: No, the code in the USB tree will not find
> its way into mainline. The opposite will happen: The mainline code will
> land in the USB tree. Which means that yes, there is a need to submit a
> patch. You can go ahead and write this up for submission, or I can
> submit it for you. Or you can check with Andy and see if he wants to
> fix the problem in a different way.
>
> Alan Stern
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-15 1:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-08 15:39 [PATCH] USB: core: Fix a NULL pointer dereference Yuran Pereira
2023-09-08 16:42 ` Alan Stern
[not found] ` <AM9P192MB12670D185D208AFA51B8348EE8ECA@AM9P192MB1267.EURP192.PROD.OUTLOOK.COM>
2023-09-09 14:36 ` Alan Stern
2023-09-09 15:35 ` gregkh
[not found] ` <AS8P192MB1269A9732001D142F3272ACDE8F6A@AS8P192MB1269.EURP192.PROD.OUTLOOK.COM>
2023-09-15 1:42 ` Alan Stern
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.