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]

Message-ID: <xa1tposjog25.fsf@mina86.com>
Date:	Wed, 18 May 2016 11:45:22 +0200
From:	Michal Nazarewicz <mina86@...a86.com>
To:	"Du\, Changbin" <changbin.du@...el.com>,
	Felipe Balbi <felipe.balbi@...ux.intel.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

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.

> 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.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»