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:	Fri, 13 May 2016 10:32:49 +0000
From:	"Du, Changbin" <changbin.du@...el.com>
To:	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Al Viro <viro@...iv.linux.org.uk>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"mina86@...a86.com" <mina86@...a86.com>,
	"rui.silva@...aro.org" <rui.silva@...aro.org>,
	"k.opasiak@...sung.com" <k.opasiak@...sung.com>,
	"lars@...afoo.de" <lars@...afoo.de>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: gadget: f_fs: report error if excess data received

Hi,

> >> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending
> >> more data than it should, is another problem altogether which needs to
> >> be addressed at the host.
> >>
> >> Adding a print to aid debugging is a good idea, but bailing out on the
> >> peripheral side is not :-s
> >>
> > Ok, if we think this is a problem at host side that the transfer is not device
> > expected, then device side should not accept the data or deliver the
> > transferred data to userspace. But now we take part of the data to
> userspace
> > and says it is ok.
> > Do you agree with this point?
> 
> We deliver to userspace the part userspace requested, right? So that's
> okay. The USB details WRT e.g. babble or host trying to send more data
> than expected, needs to be handled within the kernel.
> 
*babble* error is a good example. As you know, when xhci received more
data than expected, how does it process it? It doesn't deliver to software
the part userspace requested, but just give an error. And the xhci driver
set the urb status to -EOVERFLOW. This is similar to device side between
kernel and userspace.

> > IMO, we expose usb transfer as a file on device side. But file read() doesn't
> > have a requirement that "sorry, you cannot read so little! you need read all
> > once, else we may drop data for you. :) ".
> 
> but that's not how read() semantics work. When userspace asks to read(x)
> bytes, we have three possible outcomes:
> 
> i. We have x bytes to return, so we copy_to_user(x)
> 
> ii. We have y < x bytes to return, so we copy_to_user(y)
> 
> iii. We have y > x bytes to return, so we copy_to_user(x)
> 
I totally agree with these. They are all right. But what if userspace read
Twice? EG. When it want read a its packet, it may first read a head size,
Then read body.
read(20) = read(5) + read(15)
If this normal for a file, and works well, right? But if it happens on 
FunctionFS, the first read success, but the remailing 15 bytes lost because
they dropped by kernel. You may think it is host's bug, which also means
FunctionFS file does be different. You can compare usb with network,
Does network driver drop data? Afaik, You can read any bytes from a
socket, and data never lost.

For example, the host may send "aaabbb" and "cccddd", and device
side app May get "aaaccc" and all read success! Host send also success! 
But "bbb" "ddd" are lost.

The different views we have is on how to process the extra data. So we
can focus discussion here.

> This is exactly how the kernel is behaving. The only "detail" we have is
> that, for some reason, host is sending too much data. what I still don't
> know is if this extra data is garbage or something userspace genuinely
> cares about. Do you know the answer to this?
> 
IMO, FunctionFS should not care about what the data is. That is userspace' s
logic and meaningless for kernel. Kernel should not assume the data is garbage
or not. Is it right?

PS, for adb, it will re-open FunctionFS file after it detect unexpected data.

> > And some library that may retry read() until get enough data (which is
> > normal For a general read). Then sometimes the buffer size for
> > sys_read may not as expected. This is why I think ioctl approach is
> > more appropriate for usb transfer.
> 
> no, this won't change anything. Besides, it's a pointless discussion as
> cannot break userspace ABI. GadgetFS and FunctionFS have been shipping
> in kernel for many years.
> 
That because developers know the special requirement of FunctionFS,
just like adb.

> --
> Balbi

Do you mind if I modify my original patch for print error instead of return
error?

Best Regards,
Du, Changbin