[PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Chuansheng Liu]

When the request length is aligned to maxpacketsize, sometimes
the return length ret > the user space requested len.

At that time, we will use min_t(size_t, ret, len) to limit the
size in case of user data buffer overflow.

But we need return the min_t(size_t, ret, len) to tell the user
space rightly also.

Acked-by: Michal Nazarewicz <mina86@mina86.com>
Reviewed-by: David Cohen <david.a.cohen@linux.intel.com>
Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
---
 drivers/usb/gadget/f_fs.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 42f7a0e..780f877 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 			 * we may end up with more data then user space has
 			 * space for.
 			 */
-			ret = ep->status;
-			if (io_data->read && ret > 0 &&
-			    unlikely(copy_to_user(io_data->buf, data,
-						  min_t(size_t, ret,
-						  io_data->len))))
-				ret = -EFAULT;
+				ret = ep->status;
+				if (io_data->read && ret > 0) {
+					ret = min_t(size_t, ret, io_data->len);
+
+					if (unlikely(copy_to_user(io_data->buf,
+						data, ret)))
+						ret = -EFAULT;
+				}
 			}
 			kfree(data);
 		}
-- 
1.9.rc0

Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Tue, Mar 04, 2014 at 08:01:15PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 03/04/2014 10:34 AM, Chuansheng Liu wrote:
> 
> >When the request length is aligned to maxpacketsize, sometimes
> >the return length ret > the user space requested len.
> 
> >At that time, we will use min_t(size_t, ret, len) to limit the
> >size in case of user data buffer overflow.
> 
> >But we need return the min_t(size_t, ret, len) to tell the user
> >space rightly also.
> 
> >Acked-by: Michal Nazarewicz <mina86@mina86.com>
> >Reviewed-by: David Cohen <david.a.cohen@linux.intel.com>
> >Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> >---
> >  drivers/usb/gadget/f_fs.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> >diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> >index 42f7a0e..780f877 100644
> >--- a/drivers/usb/gadget/f_fs.c
> >+++ b/drivers/usb/gadget/f_fs.c
> >@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
> >  			 * we may end up with more data then user space has
> >  			 * space for.
> >  			 */
> >-			ret = ep->status;
> >-			if (io_data->read && ret > 0 &&
> >-			    unlikely(copy_to_user(io_data->buf, data,
> >-						  min_t(size_t, ret,
> >-						  io_data->len))))
> >-				ret = -EFAULT;
> >+				ret = ep->status;
> 
>    Why the indentation jumped suddenly to the right?

because it was wrong before ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Sergei Shtylyov]

Hello.

On 03/04/2014 10:34 AM, Chuansheng Liu wrote:

> When the request length is aligned to maxpacketsize, sometimes
> the return length ret > the user space requested len.

> At that time, we will use min_t(size_t, ret, len) to limit the
> size in case of user data buffer overflow.

> But we need return the min_t(size_t, ret, len) to tell the user
> space rightly also.

> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> Reviewed-by: David Cohen <david.a.cohen@linux.intel.com>
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> ---
>   drivers/usb/gadget/f_fs.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)

> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 42f7a0e..780f877 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
>   			 * we may end up with more data then user space has
>   			 * space for.
>   			 */
> -			ret = ep->status;
> -			if (io_data->read && ret > 0 &&
> -			    unlikely(copy_to_user(io_data->buf, data,
> -						  min_t(size_t, ret,
> -						  io_data->len))))
> -				ret = -EFAULT;
> +				ret = ep->status;

    Why the indentation jumped suddenly to the right?

> +				if (io_data->read && ret > 0) {
> +					ret = min_t(size_t, ret, io_data->len);
> +
> +					if (unlikely(copy_to_user(io_data->buf,
> +						data, ret)))
> +						ret = -EFAULT;
> +				}
>   			}
>   			kfree(data);

WBR, Sergei

Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

>> On 03/04/2014 10:34 AM, Chuansheng Liu wrote:
>> >@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
>> >  			 * we may end up with more data then user space has
>> >  			 * space for.
>> >  			 */
>> >-			ret = ep->status;
>> >-			if (io_data->read && ret > 0 &&
>> >-			    unlikely(copy_to_user(io_data->buf, data,
>> >-						  min_t(size_t, ret,
>> >-						  io_data->len))))
>> >-				ret = -EFAULT;
>> >+				ret = ep->status;

On Tue, Mar 04 2014, Felipe Balbi wrote:
>>    Why the indentation jumped suddenly to the right?

> On Tue, Mar 04, 2014 at 08:01:15PM +0300, Sergei Shtylyov wrote:
> because it was wrong before ;-)

Yep.  It looks like Robert's [2e4c7553: add aio support] introduced an
if-else-if-else flow but did not indent the code and I didn't caught it
when reviewing that patch.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Tue, Mar 04, 2014 at 08:53:40PM +0100, Michal Nazarewicz wrote:
> >> On 03/04/2014 10:34 AM, Chuansheng Liu wrote:
> >> >@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
> >> >  			 * we may end up with more data then user space has
> >> >  			 * space for.
> >> >  			 */
> >> >-			ret = ep->status;
> >> >-			if (io_data->read && ret > 0 &&
> >> >-			    unlikely(copy_to_user(io_data->buf, data,
> >> >-						  min_t(size_t, ret,
> >> >-						  io_data->len))))
> >> >-				ret = -EFAULT;
> >> >+				ret = ep->status;
> 
> On Tue, Mar 04 2014, Felipe Balbi wrote:
> >>    Why the indentation jumped suddenly to the right?
> 
> > On Tue, Mar 04, 2014 at 08:01:15PM +0300, Sergei Shtylyov wrote:
> > because it was wrong before ;-)
> 
> Yep.  It looks like Robert's [2e4c7553: add aio support] introduced an
> if-else-if-else flow but did not indent the code and I didn't caught it
> when reviewing that patch.

it's in my testing/next now, I also fixed the comment indentation which
was also wrong.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Liu, Chuansheng]

Hi Balbi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Wednesday, March 05, 2014 3:56 AM
> To: Michal Nazarewicz
> Cc: Robert Baldyga; Felipe Balbi; Sergei Shtylyov; Liu, Chuansheng;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; david.a.cohen@linux.intel.com
> Subject: Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()
> 
> On Tue, Mar 04, 2014 at 08:53:40PM +0100, Michal Nazarewicz wrote:
> > >> On 03/04/2014 10:34 AM, Chuansheng Liu wrote:
> > >> >@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file,
> struct ffs_io_data *io_data)
> > >> >  			 * we may end up with more data then user space has
> > >> >  			 * space for.
> > >> >  			 */
> > >> >-			ret = ep->status;
> > >> >-			if (io_data->read && ret > 0 &&
> > >> >-			    unlikely(copy_to_user(io_data->buf, data,
> > >> >-						  min_t(size_t, ret,
> > >> >-						  io_data->len))))
> > >> >-				ret = -EFAULT;
> > >> >+				ret = ep->status;
> >
> > On Tue, Mar 04 2014, Felipe Balbi wrote:
> > >>    Why the indentation jumped suddenly to the right?
> >
> > > On Tue, Mar 04, 2014 at 08:01:15PM +0300, Sergei Shtylyov wrote:
> > > because it was wrong before ;-)
> >
> > Yep.  It looks like Robert's [2e4c7553: add aio support] introduced an
> > if-else-if-else flow but did not indent the code and I didn't caught it
> > when reviewing that patch.
> 
> it's in my testing/next now, I also fixed the comment indentation which
> was also wrong.
Thanks your help and the fix for comment indentation also:)

Best Regards
Chuansheng

Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On Tue, Mar 04, 2014 at 11:38:32PM +0000, Liu, Chuansheng wrote:
> Hi Balbi,
> 
> > -----Original Message-----
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Wednesday, March 05, 2014 3:56 AM
> > To: Michal Nazarewicz
> > Cc: Robert Baldyga; Felipe Balbi; Sergei Shtylyov; Liu, Chuansheng;
> > gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; david.a.cohen@linux.intel.com
> > Subject: Re: [PATCH v2] usb: gadget: return the right length in ffs_epfile_io()
> > 
> > On Tue, Mar 04, 2014 at 08:53:40PM +0100, Michal Nazarewicz wrote:
> > > >> On 03/04/2014 10:34 AM, Chuansheng Liu wrote:
> > > >> >@@ -845,12 +845,14 @@ static ssize_t ffs_epfile_io(struct file *file,
> > struct ffs_io_data *io_data)
> > > >> >  			 * we may end up with more data then user space has
> > > >> >  			 * space for.
> > > >> >  			 */
> > > >> >-			ret = ep->status;
> > > >> >-			if (io_data->read && ret > 0 &&
> > > >> >-			    unlikely(copy_to_user(io_data->buf, data,
> > > >> >-						  min_t(size_t, ret,
> > > >> >-						  io_data->len))))
> > > >> >-				ret = -EFAULT;
> > > >> >+				ret = ep->status;
> > >
> > > On Tue, Mar 04 2014, Felipe Balbi wrote:
> > > >>    Why the indentation jumped suddenly to the right?
> > >
> > > > On Tue, Mar 04, 2014 at 08:01:15PM +0300, Sergei Shtylyov wrote:
> > > > because it was wrong before ;-)
> > >
> > > Yep.  It looks like Robert's [2e4c7553: add aio support] introduced an
> > > if-else-if-else flow but did not indent the code and I didn't caught it
> > > when reviewing that patch.
> > 
> > it's in my testing/next now, I also fixed the comment indentation which
> > was also wrong.
> Thanks your help and the fix for comment indentation also:)

no problem, cheers ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]