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

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