[PATCH] USB: core: Fix a NULL pointer dereference

[PATCH] USB: core: Fix a NULL pointer dereference

[Yuran Pereira]

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

Re: [PATCH] USB: core: Fix a NULL pointer dereference

[Alan Stern]

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

Re: [PATCH] USB: core: Fix a NULL pointer dereference

[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

Re: [PATCH] USB: core: Fix a NULL pointer dereference

[gregkh]

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

Re: [PATCH] USB: core: Fix a NULL pointer dereference

[Alan Stern]

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