linux-kernel - Re: [PATCH] usb: gadget: f_fs: Wake up IO thread during disconnect

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: <YaelpmsJXmhTY4A0@donbot>
Date:   Wed, 1 Dec 2021 16:41:10 +0000
From:   John Keeping <john@...anate.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>
Cc:     balbi@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        quic_jackp@...cinc.com
Subject: Re: [PATCH] usb: gadget: f_fs: Wake up IO thread during disconnect

On Wed, Dec 01, 2021 at 02:02:05AM -0800, Wesley Cheng wrote:
> During device disconnect or composition unbind, applications should be
> notified that the endpoints are no longer enabled, so that it can take
> the proper actions to handle its IO threads.  Otherwise, they can be
> left waiting for endpoints until EPs are re-enabled.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 3c584da9118c..0b0747d96378 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -957,10 +957,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
>  		if (file->f_flags & O_NONBLOCK)
>  			return -EAGAIN;
>  
> -		ret = wait_event_interruptible(
> -				epfile->ffs->wait, (ep = epfile->ep));
> +		ret = wait_event_interruptible(epfile->ffs->wait,
> +				(ep = epfile->ep) || !epfile->ffs->func);
>  		if (ret)
>  			return -EINTR;
> +		if (!epfile->ffs->func)
> +			return -ENODEV;

This seems strange - we are inside the case where the endpoint is not
initially enabled, if we're returning ENODEV here shouldn't that happen
in all cases?

Beyond that, there is no locking for accessing ffs->func here;
modification happens in gadget callbacks so it's guarded by the gadget
core (the existing case in ffs_ep0_ioctl() looks suspicious as well).

But I can't see why this change is necessary - there are event
notifications through ep0 when this happens, as can be seen in the hunk
below from the ffs_event_add(ffs, FUNCTIONFS_DISABLE) line.  If
userspace cares about this, then it can read the events from ep0.

>  	}
>  
>  	/* Do we halt? */
> @@ -3292,6 +3294,7 @@ static int ffs_func_set_alt(struct usb_function *f,
>  	if (alt == (unsigned)-1) {
>  		ffs->func = NULL;
>  		ffs_event_add(ffs, FUNCTIONFS_DISABLE);
> +		wake_up_interruptible(&ffs->wait);
>  		return 0;
>  	}
>