linux-kernel - Re: [PATCH next] usb: usbfs: Add reset

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <6419a4e9-e084-4eb6-8376-9202930ea8be@kylinos.cn>
Date: Wed, 17 Jul 2024 09:43:38 +0800
From: Hongyu Xie <xy521521@...il.com>
To: stern@...land.harvard.edu,
	xy521521@...il.com,
	oneukum@...e.com
Cc: gregkh@...uxfoundation.org,
	brauner@...nel.org,
	jlayton@...nel.org,
	jack@...e.cz,
	linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	xiehongyu1@...inos.cn
Subject: Re: [PATCH next] usb: usbfs: Add reset_resume for usbfs

From: Hongyu Xie <xiehongyu1@...inos.cn>



On 2024/7/16 20:44, Oliver Neukum wrote:
> On 15.07.24 16:22, Alan Stern wrote:
>> On Mon, Jul 15, 2024 at 10:53:14AM +0200, Oliver Neukum wrote:
>>>
>>>
>>> On 11.07.24 16:41, Alan Stern wrote:
> 
>>>> Agreed, but the solution is pretty simple.  Because the device was
>>>> suspended, the userspace driver must have enabled suspend via the
>>>> USBDEVFS_ALLOW_SUSPEND ioctl.
>>>
>>> The whole system could have been suspended, in particularly to S4.
>>
>> You are right.  I was thinking of runtime suspend, not system suspend.
>> My mistake.
> 
> This is at the intersection of several scenarios. That is a good part of
> what makes this difficult.
> In principal I think we could get away with checking for a flag to be set
> at reset_resume() before each operation. Elegant this is not. Yet, it seems
> to me like the race necessarily exists and is unsolvable in user space.
> 

 From what I know, that CONFIG_USB_DEFAULT_PERSIST is enabled by 
default. Then udev->persist_enabled is set to 1 and this causing 
udev->reset_resume set to 1 during init2 in hub_activate.
During resume,
usb_resume_both
   usb_resume_device
     generic_resume
       usb_port_resume
         finish_port_resume
           usb_reset_and_verify_device (if udev->reset_resume is 1)
             hub_port_init
               hub_port_reset
   usb_resume_interface (udev->reset_resume is 1 but usbfs doesn't have 
reset_resume implementation so set intf->needs_binding to 1, and it will 
be rebind in usb_resume_complete)

Even before usbfs->reset_resume is called (if there is one), the USB 
device has already been reset and in a good state.

After all that thaw_processes is called and user-space application runs 
again.

So I still don't understand why "the race necessarily exists". Can you 
show me an example that usbfs->reset_resume causes race?

> Furthermore in the long run, if we want to use D3cold in runtime power
> management, it looks to me like we would want a second permission ioctl
> for that.
> 
>      Regards
>          Oliver
> 
Regards
Hongyu Xie