linux-kernel - Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match

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 for Android: free password hash cracker in your pocket

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

Message-ID: <28b0a42c-2379-4844-ae18-9b0679263460@samsung.com>
Date: Mon, 2 Sep 2024 19:10:36 +0530
From: Selvarasu Ganesan <selvarasu.g@...sung.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg KH <gregkh@...uxfoundation.org>, royluo@...gle.com,
	paul@...pouillou.net, elder@...nel.org, yuanlinyu@...onor.com,
	quic_kriskura@...cinc.com, crwulff@...il.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, jh0801.jung@...sung.com,
	dh10.jung@...sung.com, naushad@...sung.com, akash.m5@...sung.com,
	rc93.raju@...sung.com, taehyun.cho@...sung.com, hongpooh.kim@...sung.com,
	eomji.oh@...sung.com, shijie.cai@...sung.com, stable <stable@...nel.org>
Subject: Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in
 gadget_match_driver


On 8/31/2024 9:59 AM, Alan Stern wrote:
> On Fri, Aug 30, 2024 at 06:16:12PM +0530, Selvarasu Ganesan wrote:
>> Hi Alan,
>>
>> Thanks for your comments. I understand your suggestions. We already have
>> a similar reference check with the udc name before calling
>> usb_gadget_register_driver.
>> In the drivers/usb/gadget/configfs.c file, I am wondering if there might
>> be an issue with the check of udc_name before
>> usb_gadget_register_driver. This is the only way to allow
>> gadget_register to be called before releasing or unregistering an
>> existing udc. Do you think we need to add an additional check here,
>> referencing the UDC, to prevent gadget_register from being called before
>> the existing UDC is released?
> I don't understand what you're saying.  There is no routine named
> "gadget_register".  (And there is no variable named "udc_name" in the
> code below, although there is gi->composite.gadget_driver.udc_name --
> but that's not a variable, it is a field in a structure.)
>
>> drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
>> ===========================================================
>> if (gi->composite.gadget_driver.udc_name) {
>>                           ret = -EBUSY;
>>                           goto err;
>>                   }
>> gi->composite.gadget_driver.udc_name = name;
> Are you talking about this check and assignment?  Why do you think there
> might be a problem here?
>
> Are you worried that some UDC might be released while this code is
> running?  If that happens, why would it be a problem?


I am talking here based on the call traces, we are observing the 
following call traces at the time of failures. One specific point of 
interest is the gadget_match_driver() function, which is called as part 
of the usb_gadget_register_driver() function. I am wondering how the 
usb_gadget_register_driver() function allows the registration of a new 
driver even when an existing same UDC is not releasing. One possibility 
is that gi->composite.gadget_driver.udc_name becomes NULL before the UDC 
is released. However, as of now, we do not have any evidence to support 
this theory. We are still trying to reproduce the same issue with added 
more debugging logs.

CPU0: (ROLE SWITCH DEVICE <-> HOST)
==================================

->usb_role_switch_set_role()
  ->dwc3_usb_role_switch_set()
   ->dwc3_set_mode()
    ->__dwc3_set_mode()
     ->dwc3_gadget_exit()
      ->usb_del_gadget()
       ->device_unregister()
        ->put_device(dev)
         ->usb_udc_release()


  CPU1 (echo "<dwc3 device name>" > <path of udc 
config>/config/usb_gadget/g1/UDC)
=================================================================================
->configfs_write_iter()
  ->gadget_dev_desc_UDC_store()
   ->usb_gadget_register_driver()
    ->driver_register()
     ->bus_add_driver()
      ->driver_attach()
       ->bus_for_each_dev()
        ->__driver_attach()
         ->gadget_match_driver()

>
>> ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
> Alan Stern
>