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:	Thu, 12 May 2016 07:30:43 +0000
From:	"Du, Changbin" <changbin.du@...el.com>
To:	Felipe Balbi <balbi@...nel.org>
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,

> >> and when has this actually happened ? Host should not send more data in
> >> this case, if it does, it's an error on the host side. Also, returning
> >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX
> >> specification of read(), right ?
> >>
> > This can happen if the host side app force kill-restart, not taking care of this
> > special condition(and we are not documented), or even it is a bug. Usually
> APPs
> > may has  a protocol to control the packet size, but protocol mismatch can
> happen
> > if either side encounter an error.
> >
> > Anyway, this is real. If kernel return success and drop data, the error may
> > explosion later, or its totally hided (but why some data lost in kernel?
> > Kernel cannot tell userspace we cannot be trusted sometimes, right?).
> > so IMO, if this is an error, we need report an error or fix it, not hide it.
> >
> > The POSIX didn't say read cannot return "-EOVERFLOW", it says:
> > " Other errors may occur, depending on the object connected to fd."
> >
> > If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions?
> >
> >> > Here, we simply report an error to userspace to let userspace
> >> > proccess. Actually, userspace applications should negotiate
> >>
> >> no, this violates POSIX. Care to explain what problem are you actually
> >> facing ?
> >>
> > Why this violates POSIX? Could you give more details?
> 
> read(5) should return at mode 5 bytes. If there are more, than 5 bytes,
> we don't error out, we just return the requested 5 bytes and wait for a
> further read.
> 
Yes, it is true. As I mentioned before, we also can keep the extra data for
next read. This need more work to maintain a buffer. Here I just simply 
report an error(let userspace know something goes wrong.) before the
logic is implemented by someone.
(Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)

> What I'm more concerned, however, is why we received more than
> expected
> data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps
> add a few traces doing a hexdump (using __print_hex()) of the data in
> req->buf.
> 
The extra bytes can be anything(random), they just data from APP layer.
It doesn't make sense for you to check. So I will not dump them, sorry.

> > The problem is device side app sometimes received incorrect data caused
> > by the dropping. Most times the error can be detected by APP itself, but
> 
> why ? app did e.g. read(5), that caused driver to queue a usb_request
> with length set to 512. Host sent more data than the expected 5 bytes,
> why did host do that ? And if that data was needed, why didn't userspace
> read() more than 5 ?
> 
> --
> Balbi
Well, first, there must be a protocol upon usb between host side and device side.
Second device side didn't know how many bytes to receive, it need host side tell
it.  But host could be buggy, or the application is killed and restart. These all can lead
host send more than device wanted bytes. For sure it wrong at host side, but device
side don't know.

Best Regards,
Du, Changbin