linux-kernel - Re: [PATCH] usb: gadget: f_fs: report error if excess data received

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:	Wed, 18 May 2016 13:15:13 +0300
From:	Felipe Balbi <felipe.balbi@...ux.intel.com>
To:	Michal Nazarewicz <mina86@...a86.com>,
	"Du\, Changbin" <changbin.du@...el.com>,
	Alan Stern <stern@...land.harvard.edu>
Cc:	Al Viro <viro@...iv.linux.org.uk>,
	"gregkh\@linuxfoundation.org" <gregkh@...uxfoundation.org>,
	"rui.silva\@linaro.org" <rui.silva@...aro.org>,
	"k.opasiak\@samsung.com" <k.opasiak@...sung.com>,
	"lars\@metafoo.de" <lars@...afoo.de>,
	"linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: f_fs: report error if excess data received


Hi,

Michal Nazarewicz <mina86@...a86.com> writes:
> On Tue, May 17 2016, Changbin Du wrote:
>>> There appears to be no kfifo support for iov_iter though, so I just went
>>> with a simple buffer.
>>> 
>>> I haven’t looked at the patch too carefully so this is an RFC rather
>>> than an actual patch at this point.  It does compile at least.
>>> 
>>> Regardless, the more I thin about it, the more I’m under the impression
>>> that the whole rounding up in f_fs was a mistake.  And the more I’m
>>> leaning towards ignoring the excess data set by the host.
>>> 
>>> ---------- >8 ----------------------------------------------------------
>>> Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests
>>> 
>>> f_fs rounds up read(2) requests to a multiple of a max packet size
>>> which means that host may provide more data than user has space for.
>>> So far, the excess data has been silently ignored.
>>> 
>>> This introduces a buffer for a tail of such requests so that they are
>>> returned on next read instead of being ignored.
>>> 
>>
>> Congratulations finally reach an agreement,
>
> To be honest, if it was up to me, I would rip request size rounding up
> out of the code.

we've been through this before. This needs to be done at the gadget
layer. Gadget driver can over-allocate ahead of time if
gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at the
UDC driver level.

>> thanks Alan Stern and Michal.
>> Here just have a comment - the buffered data need be dropped when the
>> epfile is closed, because it means the session is terminated.
>
> I blame that on sleep deprivation.  Another issue is what to do when
> endpoint is disabled.  Should the buffer be cleared as soon as the
> endpoint is disabled?  Or maybe when the endpoint is enabled again?  Or
> maybe it should never be cleared?
>
> If the buffer is cleared when endpoint is disabled, we again silently
> drop data.  On the other hand, if we don’t do that, read() on the
> endpoint will may succeed even if the configuration is disabled which
> may be surprising for users.

tough decision... but seems like clearing the buffer as soon as ep is
disabled is the way to go.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)