| Message ID | AS8P192MB12697886EC8DF1650AD56A57E8EDA@AS8P192MB1269.EURP192.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: core: Fix a NULL pointer dereference | expand |
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
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
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
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
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) {
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(-)