linux-kernel - Re: [PATCH v2 24/25] usb: gadget: f

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]

Date:   Fri, 26 Aug 2022 19:27:49 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Dmitry Bogdanov <d.bogdanov@...ro.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "target-devel@...r.kernel.org" <target-devel@...r.kernel.org>
Subject: Re: [PATCH v2 24/25] usb: gadget: f_tcm: Check overlapped command

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 8b99ee07df87..dfa3e7c043a3 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
> >  	return;
> >  
> >  skip:
> > +	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > +		struct se_session *se_sess;
> > +		struct uas_stream *stream;
> > +
> > +		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > +		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> > +
> > +		/*
> > +		 * There's no guarantee of a matching completion order between
> > +		 * different endpoints. i.e. The device may receive a new (CDB)
> > +		 * command request completion of the command endpoint before it
> > +		 * gets notified of the previous command status completion from
> > +		 * a status endpoint. The driver still needs to detect
> > +		 * misbehaving host and respond with an overlap command tag. To
> > +		 * prevent false overlapped tag failure, give the active and
> > +		 * matching stream id a short time (1ms) to complete before
> > +		 * respond with overlapped command failure.
> > +		 */
> > +		msleep(1);
> 
> How likely is it for this to happen? Is there maybe some synchronisation
> missing? If I see this right, in order to get here, you will already
> spill the message "Command tag %d overlapped" which does not look good.
> Why should the host re-use a tag which did not yet complete?
> 

Not often, but it can happen. For example, even though the device sent a
status on the wire and the host received it. The host may send a new
command with the same tag before the device is notified of the previous
command completion. Since they operate in different endpoints, the
device driver may get notification of the new command from the command
endpoint before the status completion of the status endpoint.

This is an attempt to maintain synchronization and prevent false overlap
check. It's a quick fix. I feel that we can handle this better. If you
can think of a better way to handle this, let me know.

BR,
Thinh