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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d3ffde1a-e0da-4f3f-ac34-659cbcf41258@rowland.harvard.edu>
Date:   Fri, 8 Sep 2023 12:42:16 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Yuran Pereira <yuran.pereira@...mail.com>
Cc:     gregkh@...uxfoundation.org, royluo@...gle.com,
        christophe.jaillet@...adoo.fr, raychi@...gle.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+c063a4e176681d2e0380@...kaller.appspotmail.com
Subject: Re: [PATCH] USB: core: Fix a NULL pointer dereference

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@...kaller.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