linux-kernel - Re: [PATCH] usb: storage: realtek_cr: get lock before calling usb_enable

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]

Date:   Mon, 2 Aug 2021 20:56:28 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Salah Triki <salah.triki@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, usb-storage@...ts.one-eyed-alien.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: storage: realtek_cr: get lock before calling
 usb_enable_autosuspend()

On Tue, Aug 03, 2021 at 12:03:13AM +0100, Salah Triki wrote:
> Based on the documentation of usb_enable_autosuspend(), the
> caller must hold udev's device lock.
> 
> Signed-off-by: Salah Triki <salah.triki@...il.com>
> ---
>  drivers/usb/storage/realtek_cr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index 3789698d9d3c..6948d6fdad39 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -918,9 +918,13 @@ static int realtek_cr_autosuspend_setup(struct us_data *us)
>  	timer_setup(&chip->rts51x_suspend_timer, rts51x_suspend_timer_fn, 0);
>  	fw5895_init(us);
>  
> +	usb_lock_device(us->pusb_dev);
> +
>  	/* enable autosuspend function of the usb device */
>  	usb_enable_autosuspend(us->pusb_dev);
>  
> +	usb_unlock_device(us->pusb_dev);
> +
>  	return 0;
>  }
>  #endif

I assume you didn't test this patch, because RTS51xx card readers aren't 
very common.  If you had tested it, you would have found that it causes 
the system to hang.

This is because when realtek_cr_autosuspend_setup runs, the driver 
already holds the device lock.  The route is a little circuitous:

	realtek_cr_probe is called with the lock held, like all
	probe routines.

	It calls usb_store_probe2, which calls 
	usb_stor_acquire_resources.  That routine calls 
	us->unusual_dev->initFunction.  For the realtek_cr driver, the 
	initFunction is set to init_realtek_cr (see unusual_realtek.h).

	init_realtek_cr calls realtek_cr_autosuspend_setup.

There is no other pathway to the usb_enable_autosuspend call.  So your 
patch is not needed.

Alan Stern