Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1398925 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: f_fs: report error if excess data received changbin.du@intel.com - 2016-05-11 12:40 +0200
    Re: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <balbi@kernel.org> - 2016-05-11 13:10 +0200
      Re: [PATCH] usb: gadget: f_fs: report error if excess data received Michal Nazarewicz <mina86@mina86.com> - 2016-05-11 14:40 +0200
        RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 06:30 +0200
      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 06:30 +0200
        RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <balbi@kernel.org> - 2016-05-12 09:00 +0200
          RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 09:40 +0200
            RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 09:50 +0200
              RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 10:20 +0200
                RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 11:20 +0200
                  RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 11:30 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 12:00 +0200
                  RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 11:50 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 12:20 +0200
                    RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 12:20 +0200
                      RE: [PATCH] usb: gadget: f_fs: report error if excess data received "Du, Changbin" <changbin.du@intel.com> - 2016-05-12 12:50 +0200
                        RE: [PATCH] usb: gadget: f_fs: report error if excess data received Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-05-12 13:30 +0200

#1398925 — [PATCH] usb: gadget: f_fs: report error if excess data received

From: "Du, Changbin" <changbin.du@intel.com>

Since the buffer size for req is rounded up to maxpacketsize,
then we may end up with more data then user space has space
for.

If it happen, we can keep the excess data for next i/o, or
report an error. But we cannot silently drop data, because
USB layer should ensure the data integrality it has transferred,
otherwise applications may get corrupt data if it doesn't
detect this case.

Here, we simply report an error to userspace to let userspace
proccess. Actually, userspace applications should negotiate
with host side for how many bytes it should receive.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/gadget/function/f_fs.c | 48 +++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 15b648c..411ed2d 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -640,6 +640,36 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
 	}
 }
 
+static size_t ffs_copy_to_user(const void *buf, size_t bytes,
+				struct ffs_io_data *io_data)
+{
+	size_t count = iov_iter_count(&io_data->data);
+	int ret;
+
+	/**
+	 * Since the buffer size for req is rounded up to maxpacketsize,
+	 * then we may end up with more data then user space has space for.
+	 * We can keep the excess data for next i/o, or report an error.
+	 * But we cannot silently drop data, because USB layer should ensure
+	 * the data integrality it has transferred.
+	 *
+	 * Here, we simply report an error to userspace to let userspace
+	 * proccess. Actually, userspace applications should negotiate with
+	 * each other for how many bytes host send.
+	 */
+	if (bytes > count) {
+		pr_err("ffs read size %zu bigger than requested size %zu\n",
+			bytes, count);
+		return -EOVERFLOW;
+	}
+
+	ret = copy_to_iter(buf, bytes, &io_data->data);
+	if (ret != bytes)
+		return -EFAULT;
+
+	return ret;
+}
+
 static void ffs_user_copy_worker(struct work_struct *work)
 {
 	struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
@@ -650,9 +680,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
 
 	if (io_data->read && ret > 0) {
 		use_mm(io_data->mm);
-		ret = copy_to_iter(io_data->buf, ret, &io_data->data);
-		if (iov_iter_count(&io_data->data))
-			ret = -EFAULT;
+		ret = ffs_copy_to_user(io_data->buf, ret, io_data);
 		unuse_mm(io_data->mm);
 	}
 
@@ -803,18 +831,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 			interrupted = ep->status < 0;
 		}
 
-		/*
-		 * XXX We may end up silently droping data here.  Since data_len
-		 * (i.e. req->length) may be bigger than len (after being
-		 * rounded up to maxpacketsize), we may end up with more data
-		 * then user space has space for.
-		 */
 		ret = interrupted ? -EINTR : ep->status;
-		if (io_data->read && ret > 0) {
-			ret = copy_to_iter(data, ret, &io_data->data);
-			if (!ret)
-				ret = -EFAULT;
-		}
+		if (io_data->read && ret > 0)
+			ret = ffs_copy_to_user(data, ret, io_data);
+
 		goto error_mutex;
 	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
 		ret = -ENOMEM;
-- 
2.7.4

[toc] | [next] | [standalone]

#1398946

[Multipart message — attachments visible in raw view] — view raw

Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> Since the buffer size for req is rounded up to maxpacketsize,
> then we may end up with more data then user space has space
> for.

only for OUT direction with the controller you're using ;-)

> If it happen, we can keep the excess data for next i/o, or
> report an error. But we cannot silently drop data, because
> USB layer should ensure the data integrality it has transferred,
> otherwise applications may get corrupt data if it doesn't
> detect this case.

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 ?

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

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1399022

On Wed, May 11 2016, Felipe Balbi wrote:
> Also, returning -EOVERFLOW is not exactly correct here, because you'd
> violate POSIX specification of read(), right ?

Maybe we could piggyback on:

       EINVAL fd was created via a call to timerfd_create(2) and the
              wrong size buffer was given to read();

But I kinda agree.  I’m not sure how much we need to care about this
instead of having user space round their buffers up to the nearest max
packet size boundary.

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

[toc] | [prev] | [next] | [standalone]

#1399664

> On Wed, May 11 2016, Felipe Balbi wrote:
> > Also, returning -EOVERFLOW is not exactly correct here, because you'd
> > violate POSIX specification of read(), right ?
> 
> Maybe we could piggyback on:
> 
>        EINVAL fd was created via a call to timerfd_create(2) and the
>               wrong size buffer was given to read();
> 
> But I kinda agree.  I’m not sure how much we need to care about this
> instead of having user space round their buffers up to the nearest max
> packet size boundary.
> 
> --
> Best regards
> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
> «If at first you don’t succeed, give up skydiving»

This is a good idea that "having user space round their buffers". But kernel
Still cannot hide error silently. :)

[toc] | [prev] | [next] | [standalone]

#1399663

> Hi,
> 
> changbin.du@intel.com writes:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > Since the buffer size for req is rounded up to maxpacketsize,
> > then we may end up with more data then user space has space
> > for.
> 
> only for OUT direction with the controller you're using ;-)
For sure.

> 
> > If it happen, we can keep the excess data for next i/o, or
> > report an error. But we cannot silently drop data, because
> > USB layer should ensure the data integrality it has transferred,
> > otherwise applications may get corrupt data if it doesn't
> > detect this case.
> 
> 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?

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
sometimes cannot. It depends on the design of its communication protocol.

> --
> Balbi

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1399698

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> > If it happen, we can keep the excess data for next i/o, or
>> > report an error. But we cannot silently drop data, because
>> > USB layer should ensure the data integrality it has transferred,
>> > otherwise applications may get corrupt data if it doesn't
>> > detect this case.
>> 
>> 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.

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

[toc] | [prev] | [next] | [standalone]

#1399719

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

[toc] | [prev] | [next] | [standalone]

#1399721

Hi,

"Du, Changbin" <changbin.du@intel.com> 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

[toc] | [prev] | [next] | [standalone]

#1399742

> > 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 :-)
> 
Do you really want a random hex string? I don't think it is useful.

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

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

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

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

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1399815

Hi,

"Du, Changbin" <changbin.du@intel.com> 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

[toc] | [prev] | [next] | [standalone]

#1399821

Hi again,

Felipe Balbi <felipe.balbi@linux.intel.com> writes:
> @@ -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);

we need a min() here. Better version below:

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d54e1cc..6c49b152f46e 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,15 @@ 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);
+			ssize_t bytes;
+
+			if (ret > io_data->expected_len)
+				pr_debug("FFS: size mismatch: %zd for %zd",
+						ret, io_data->expected_len);
+
+			bytes = min(ret, io_data->expected_len);
+
+			ret = copy_to_iter(data, bytes, &io_data->data);
 			if (!ret)
 				ret = -EFAULT;
 		}


-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1399845

Hi,
> 
> we need a min() here. Better version below:
No need. copy_to_iter will do it for us.

Best Regards,
Du, Changbin

> 
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..6c49b152f46e 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,15 @@ 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);
> +			ssize_t bytes;
> +
> +			if (ret > io_data->expected_len)
> +				pr_debug("FFS: size mismatch: %zd for %zd",
> +						ret, io_data->expected_len);
> +
> +			bytes = min(ret, io_data->expected_len);
> +
> +			ret = copy_to_iter(data, bytes, &io_data->data);
>  			if (!ret)
>  				ret = -EFAULT;
>  		}
> 
> 
> --
> balbi

[toc] | [prev] | [next] | [standalone]

#1399834

> >> > 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?
> 
For sure, maybe I didn't describe it well so let you confused. :)

> 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:
> 
No, the problem is we drop data but silently. We cannot give more data to
userspace since buffer is limited.

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

The different for this code is just give warning but not return error. It is also
fine for me that at least this let development can find some key message to
find What happed under kernel. But the message should be *error* I think.

And this missed AIO path. This is identify to my patch after remove the
"return -EOVERFLOW;" line.

Byw, we not need add the field "expected_len", we can get it from the
struct ffs_io_data.

If this is fine for you, I can publish a new patch.

> --
> Balbi

Best Regards,
Du, Changbin

[toc] | [prev] | [next] | [standalone]

#1399861

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> >> > 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?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> 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:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -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").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1399862

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> >> > 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?
>> 
> For sure, maybe I didn't describe it well so let you confused. :)

okay

>> 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:
>> 
> No, the problem is we drop data but silently. We cannot give more data to

okay, but does that create any problems for device side userspace? What
problem is that?

> userspace since buffer is limited.

right, and that was my point: if we copy more to userspace, then we have
a real big problem.

>> @@ -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").
>> 
>
> The different for this code is just give warning but not return
> error. It is also fine for me that at least this let development can
> find some key message to find What happed under kernel. But the
> message should be *error* I think.

I'm fine with pr_error()

> And this missed AIO path. This is identify to my patch after remove the

right, it's more of a debug patch since I don't have the setup to
trigger this (I'm assuming you're using adb?)

> "return -EOVERFLOW;" line.

there's one key difference, see below

> Byw, we not need add the field "expected_len", we can get it from the
> struct ffs_io_data.

without expected_len we can copy more data to userspace, right ? If
req->actual > data_len_before_aligning_to_maxpacket, then we will copy
more data then we should to userspace and this was a regression caused
by Al's commit, AFAICT.

> If this is fine for you, I can publish a new patch.
>
>> --
>> Balbi
>
> Best Regards,
> Du, Changbin

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1399908

> Hi,
> 
> "Du, Changbin" <changbin.du@intel.com> writes:
> >> >> > 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?
> >>
> > For sure, maybe I didn't describe it well so let you confused. :)
> 
> okay
> 
> >> 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:
> >>
> > No, the problem is we drop data but silently. We cannot give more data to
> 
> okay, but does that create any problems for device side userspace? What
> problem is that?
> 
> > userspace since buffer is limited.
> 
> right, and that was my point: if we copy more to userspace, then we have
> a real big problem.
> 
Yes, we drop the data because we userspace buffer is not enough this time.
The problem here is that really can we just drop it silently? Maybe not.

> >> @@ -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").
> >>
> >
> > The different for this code is just give warning but not return
> > error. It is also fine for me that at least this let development can
> > find some key message to find What happed under kernel. But the
> > message should be *error* I think.
> 
> I'm fine with pr_error()
> 
> > And this missed AIO path. This is identify to my patch after remove the
> 
> right, it's more of a debug patch since I don't have the setup to
> trigger this (I'm assuming you're using adb?)
> 
Right. And adb can detect this unexpected behavior(data mismatch) quickly
because it has some selfcheck for the data content.

> > "return -EOVERFLOW;" line.
> 
> there's one key difference, see below
> 
> > Byw, we not need add the field "expected_len", we can get it from the
> > struct ffs_io_data.
> 
> without expected_len we can copy more data to userspace, right ? If
> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
> more data then we should to userspace and this was a regression caused
> by Al's commit, AFAICT.
> 
No, expected_len equals to iov_iter_count(&io_data->data), right? So we
do not need a new field.

> > If this is fine for you, I can publish a new patch.
> >
> >> --
> >> Balbi
> >
> > Best Regards,
> > Du, Changbin
> 
> --
> balbi

[toc] | [prev] | [next] | [standalone]

#1399935

Hi,

"Du, Changbin" <changbin.du@intel.com> writes:
>> right, and that was my point: if we copy more to userspace, then we have
>> a real big problem.
>> 
> Yes, we drop the data because we userspace buffer is not enough this time.
> The problem here is that really can we just drop it silently? Maybe not.

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

>> > And this missed AIO path. This is identify to my patch after remove the
>> 
>> right, it's more of a debug patch since I don't have the setup to
>> trigger this (I'm assuming you're using adb?)
>> 
> Right. And adb can detect this unexpected behavior(data mismatch) quickly
> because it has some selfcheck for the data content.

cool

>> > Byw, we not need add the field "expected_len", we can get it from the
>> > struct ffs_io_data.
>> 
>> without expected_len we can copy more data to userspace, right ? If
>> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
>> more data then we should to userspace and this was a regression caused
>> by Al's commit, AFAICT.
>> 
> No, expected_len equals to iov_iter_count(&io_data->data), right? So we
> do not need a new field.

/me goes read iov_iter_count()

you're right, we don't need expected len at all ;-)

in any case, did you figure out if the extra data host sends is
important data at all or just garbage ?

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web