Re: [PATCH v2] usbip: usbip_host: fix null pointer dereference in

[Jeffin Philip]

On Fri, Aug 07, 2026 at 03:09:56PM +0200, Greg KH wrote:
>On Fri, Aug 07, 2026 at 03:48:22PM +0530, Jeffin Philip wrote:
>> On Fri, Aug 07, 2026 at 08:00:31 +0200, Greg KH wrote:
>> >On Fri, Aug 07, 2026 at 10:07:59AM +0530, Jeffin Philip wrote:
>> >> rebind_store calls do_rebind which dereferences udev without
>> >> checking if it is NULL. If busid is registered using match_busid
>> >> but the device is never bound to the driver or it is never present
>> >> in the first place, it triggers a null pointer dereference when we
>> >> attempt to rebind the device. Fix this by checking explicitly for
>> >> udev first and returning -ENODEV if udev is NULL. Add usb_get_dev
>> >> in addition to the null check to get a reference to udev to prevent
>> >> udev from becoming NULL after the check. Drop the reference after
>> >> using it in do_rebind().
>> >> 
>> >> Reported-by: syzbot+af76b01c9a0f0ab60fb0@xxxxxxxxxxxxxxxxxxxxxxxxx
>> >> Closes: https://syzkaller.appspot.com/bug?extid=af76b01c9a0f0ab60fb0
>> >> Fixes: 4bfb141bc013 ("usbip: usbip_host: fix to hold parent lock for device_attach() calls")
>> >> Cc: stable@xxxxxxxxxxxxxxx
>> >> Signed-off-by: Jeffin Philip <jeffinphilip14@xxxxxxxxx>
>> >> ---
>> >> Changes in v2:
>> >>  - Addressed concerns raised by the maintainer in v1 discussion
>> >>    - Added usb_get_dev() to get a reference to udev preventing
>> >>      it from becoming null after the null check. Drop the reference
>> >>      after using it in do_rebind()
>> >> ---
>> >>  drivers/usb/usbip/stub_main.c | 9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >> 
>> >> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
>> >> index 79110a69d697..c911626427dc 100644
>> >> --- a/drivers/usb/usbip/stub_main.c
>> >> +++ b/drivers/usb/usbip/stub_main.c
>> >> @@ -256,12 +256,21 @@ static ssize_t rebind_store(struct device_driver *dev, const char *buf,
>> >>  	if (!bid)
>> >>  		return -ENODEV;
>> >>  
>> >> +	if (!bid->udev) {
>> >> +		put_busid_priv(bid);
>> >> +		return -ENODEV;
>> >> +	}
>> >> +
>> >> +	/* get a reference to udev to prevent it from becoming NULL */
>> >> +	usb_get_dev(bid->udev);
>> >
>> >So what happens if udev becomes NULL after checking it and before
>> >grabbing the reference?
>> 
>> I looked through the driver and could not find where udev becomes NULL after
>> checking and before getting the reference since we do both of those under
>> busid lock. There is a small window between releasing the lock and
>> do_rebind, is that what what you are referring to? If so or otherwise, 
>> could you please advise me on how to move forward in correcting this patch?
>
>If you do not hold a lock when testing and doing something based on a
>field, it will race and is broken.
>
>Again, step back and try to determine what you are trying to fix here,
>and how that can be done in a race-free way.  If you don't know, that's
>fine too, I sure don't!  :)

We could add a new flag(in bus_id_priv->status) that would prevent probe from
scanning or any other function from nullifying udev and that would prevent race
conditions. But I think that would be an unnecessarily large change just for a
small issue like null udev. We could also use a local variable to store the udev
so it does not become null after dropping locks. Apart from these, I don't have 
other ideas.

>But we can't take a change that doesn't actually fix the issue, you
>wouldn't want that, right?

Yes, I don't want to send bad patches either. 

>Why are you looking at this issue anyway?  Did someone assign it to you?

I am relatively new to usb subsystem and have been wanting to learn it for
quite some time. So I got on syzbot where I found this issue, I wanted to
learn the subsystem in practice, that's all. Nobody assigned me this.

Thanks,
Jeffin.