RE: [PATCH] usb: gadget: f_fs: report error if excess data received

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@xxxxxxxxx> writes:
> 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.

no, this is not how we do things here. If we find a bug we actually fix
it, we don't just work around it ;-)

> (Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.)

heh, no :-)

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

interesting, so you claim to have found a bug, but when asked to provide
more information your answer is "no" ? Thanks :-)

>> > 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 ?
>> 
>
> Well, first, there must be a protocol upon usb between host side and
> device side.

sorry, I don't know what mean here. USB does not *require* a protocol
running on top of USB. There usually is one, but that's not a
requirement.

> Second device side didn't know how many bytes to receive, it need host
> side tell it.

well, many protocols work like this. See Mass Storage, for example.

> But host could be buggy,

if host is buggy, why should we fix host on the peripheral side ?

> or the application is killed and restart.

If application is killed (why was the application killed? Which
application was killed?), then why are we still connected to host at
all? It's clear that this gadget can't work without its userspace
counterpart. If that userspace isn't available, we should drop data
pullup and disconnect from host.

> These all can lead host send more than device wanted bytes. For sure
> it wrong at host side, but device side don't know.

but none of this means we have a bug at device side. In fact, by
allowing these extra bytes to reach userspace, we could be creating a
possible attack vector.

Your explanation is unsatisfactory, so I won't apply your patch, sorry.

-- 
balbi