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

[Felipe Balbi]

Hi,

"Du, Changbin" <changbin.du@xxxxxxxxx> writes:
>> >> > 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.
>> 
> Communication between two endpoints must has a protocol, even it may
> very simple. Without protocol, they cannot exchange information.

that protocol is USB :-)

>> > 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 ?
>> 
> True it is bug of host, but is it a reason kernel can drop data then? 

sure is :-) For all we know, that data means nothing to us

>> > 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.
>> 
> Usb no need disconnect if the application exit (host side). Seems you
> only care about device side.

oh, application was killed on host side. I thought it was on device
side. Yeah, then we shouldn't disconnect.

>> > 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
> It is fine. Then need userspace take care of all the data it received. Because
> Kernel may drop some data for it. Kernel ffs driver is unauthentic sometimes.

I really cannot understand what you mean sometimes. You're saying that
userspace needs to take care of all the data it received because kernel
can drop data. If kernel is dropping data, there's no extra data
reaching userspace, right?

Is the problem that we *are* giving more data than expected to
userspace? Are we overflowing some userspace buffer? If that's the case,
then below should be enough for the time being:

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..d1bd53c895ca 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -156,6 +156,8 @@ struct ffs_io_data {
 	struct usb_request *req;
 
 	struct ffs_data *ffs;
+
+	ssize_t expected_len;
 };
 
 struct ffs_desc_helper {
@@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * Controller may require buffer size to be aligned to
 		 * maxpacketsize of an out endpoint.
 		 */
-		if (io_data->read)
+		if (io_data->read) {
+			io_data->expected_len = data_len;
 			data_len = usb_ep_align_maybe(gadget, ep->ep, data_len);
+		}
 		spin_unlock_irq(&epfile->ffs->eps_lock);
 
 		data = kmalloc(data_len, GFP_KERNEL);
@@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 */
 		ret = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
-			ret = copy_to_iter(data, ret, &io_data->data);
+			if (ret > io_data->expected_len)
+				pr_debug("FFS: size mismatch: %zd for %zd",
+						ret, io_data->expected_len);
+
+			ret = copy_to_iter(data, io_data->expected_len,
+					&io_data->data);
 			if (!ret)
 				ret = -EFAULT;
 		}

that we can get merged during v4.7-rc and Cc stable and backport this to
anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
use put iov_iter into io_data").

-- 
balbi