Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Hi Michal, On 11/08/2013 04:23 AM, Michal Nazarewicz wrote: On Thu, Nov 07 2013, Alan Stern wrote: What happens if the userspace daemon writes to epfile but the host changes the config or altsetting before all the data can be sent? Does the remaining data get flushed? Each read and write is mapped to a single request, so the usual. I'm still a little unclear on this. Disabling the function ought to have much the same effect as changing the config or altsetting: Writes to endpoint files should be flushed and reads should be terminated. Otherwise you would end up sending stale data to the host or reading data that the daemon isn't prepared for. You may have a point here. I'll try to prepare a patch over the weekend. It looks like our patches are going to have dependence. If you send yours this weekend, I'll wait to send new version of this patch of mine on top of yours. Br, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Thu, Nov 07 2013, Alan Stern wrote: > What happens if the userspace daemon writes to epfile but the host > changes the config or altsetting before all the data can be sent? Does > the remaining data get flushed? Each read and write is mapped to a single request, so the usual. > I'm still a little unclear on this. Disabling the function ought to > have much the same effect as changing the config or altsetting: Writes > to endpoint files should be flushed and reads should be terminated. > Otherwise you would end up sending stale data to the host or reading > data that the daemon isn't prepared for. You may have a point here. I'll try to prepare a patch over the weekend. > Given that, there doesn't seem to be any need for a loop. Copy the > data; if the function was disabled in the meantime then throw away the > data and return an appropriate error code. Right. > I don't see any reason why you should ever have to copy the same data > from userspace multiple times. That actually never happens. The data is copied at most once. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Thu, Nov 07 2013, Alan Stern wrote: What happens if the userspace daemon writes to epfile but the host changes the config or altsetting before all the data can be sent? Does the remaining data get flushed? Each read and write is mapped to a single request, so the usual. I'm still a little unclear on this. Disabling the function ought to have much the same effect as changing the config or altsetting: Writes to endpoint files should be flushed and reads should be terminated. Otherwise you would end up sending stale data to the host or reading data that the daemon isn't prepared for. You may have a point here. I'll try to prepare a patch over the weekend. Given that, there doesn't seem to be any need for a loop. Copy the data; if the function was disabled in the meantime then throw away the data and return an appropriate error code. Right. I don't see any reason why you should ever have to copy the same data from userspace multiple times. That actually never happens. The data is copied at most once. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--m...@google.com--xmpp:min...@jabber.org--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Hi Michal, On 11/08/2013 04:23 AM, Michal Nazarewicz wrote: On Thu, Nov 07 2013, Alan Stern wrote: What happens if the userspace daemon writes to epfile but the host changes the config or altsetting before all the data can be sent? Does the remaining data get flushed? Each read and write is mapped to a single request, so the usual. I'm still a little unclear on this. Disabling the function ought to have much the same effect as changing the config or altsetting: Writes to endpoint files should be flushed and reads should be terminated. Otherwise you would end up sending stale data to the host or reading data that the daemon isn't prepared for. You may have a point here. I'll try to prepare a patch over the weekend. It looks like our patches are going to have dependence. If you send yours this weekend, I'll wait to send new version of this patch of mine on top of yours. Br, David Cohen -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Wed, 6 Nov 2013, Michal Nazarewicz wrote: > On Tue, Nov 05 2013, Alan Stern wrote: > > Maybe Michal can enlighten us. > > Sorry for late response, this thread fell under my radar for some > reason. > > So here's how it works: > > epfile represents an end point file on the fuctionfs file system, > i.e. what user space is seeing. It's numbering is independent of which > USB configuration is chosen. > > A FunctionFS user space daemon may read/write to those files regardless > of whether the function is enabled. If it is not, the operation will > block until host enables the function. What happens if the userspace daemon writes to epfile but the host changes the config or altsetting before all the data can be sent? Does the remaining data get flushed? Similarly, what happens if the config or altsetting changes before a read of epfile completes? Does the read get terminated? > epfile->ep represents an actual USB end point, and it's number does not > have to correspond to the epfile file name, and may be different in > different configuration. FunctionFS hides all that from the user space > daemon. > > epfile->ep is set when host changes configuration (i.e. function's > set_alt or disable callbacks). IIRC this implies that epfile->ep cannot > be protected by a mutex, and therefore is protected by a spinlock. > > Since it is protected by a spinlock, the ffs_epfile_io function cannot > lock it and then proceed to allocating memory and copying data from user > space. That's why there is the need for the loop since there is no way > to guarantee that while the memory was allocated and data was read from > user space (if it is a write), the function has not been disabled and > re-enabled. I'm still a little unclear on this. Disabling the function ought to have much the same effect as changing the config or altsetting: Writes to endpoint files should be flushed and reads should be terminated. Otherwise you would end up sending stale data to the host or reading data that the daemon isn't prepared for. Given that, there doesn't seem to be any need for a loop. Copy the data; if the function was disabled in the meantime then throw away the data and return an appropriate error code. I don't see any reason why you should ever have to copy the same data from userspace multiple times. > However, I'm not sure whether maxpacketsize can change. It is part of > endpoint's descriptor and even though the endpoint number can change > while ffs_epfile_io is running, all the other descriptor fields should > stay the same. If the config or altsetting can change then the maxpacket size can change too. However, the reasoning above indicates that this should be moot. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Wed, 6 Nov 2013, Michal Nazarewicz wrote: On Tue, Nov 05 2013, Alan Stern wrote: Maybe Michal can enlighten us. Sorry for late response, this thread fell under my radar for some reason. So here's how it works: epfile represents an end point file on the fuctionfs file system, i.e. what user space is seeing. It's numbering is independent of which USB configuration is chosen. A FunctionFS user space daemon may read/write to those files regardless of whether the function is enabled. If it is not, the operation will block until host enables the function. What happens if the userspace daemon writes to epfile but the host changes the config or altsetting before all the data can be sent? Does the remaining data get flushed? Similarly, what happens if the config or altsetting changes before a read of epfile completes? Does the read get terminated? epfile-ep represents an actual USB end point, and it's number does not have to correspond to the epfile file name, and may be different in different configuration. FunctionFS hides all that from the user space daemon. epfile-ep is set when host changes configuration (i.e. function's set_alt or disable callbacks). IIRC this implies that epfile-ep cannot be protected by a mutex, and therefore is protected by a spinlock. Since it is protected by a spinlock, the ffs_epfile_io function cannot lock it and then proceed to allocating memory and copying data from user space. That's why there is the need for the loop since there is no way to guarantee that while the memory was allocated and data was read from user space (if it is a write), the function has not been disabled and re-enabled. I'm still a little unclear on this. Disabling the function ought to have much the same effect as changing the config or altsetting: Writes to endpoint files should be flushed and reads should be terminated. Otherwise you would end up sending stale data to the host or reading data that the daemon isn't prepared for. Given that, there doesn't seem to be any need for a loop. Copy the data; if the function was disabled in the meantime then throw away the data and return an appropriate error code. I don't see any reason why you should ever have to copy the same data from userspace multiple times. However, I'm not sure whether maxpacketsize can change. It is part of endpoint's descriptor and even though the endpoint number can change while ffs_epfile_io is running, all the other descriptor fields should stay the same. If the config or altsetting can change then the maxpacket size can change too. However, the reasoning above indicates that this should be moot. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, Nov 05 2013, Alan Stern wrote: > Maybe Michal can enlighten us. Sorry for late response, this thread fell under my radar for some reason. So here's how it works: epfile represents an end point file on the fuctionfs file system, i.e. what user space is seeing. It's numbering is independent of which USB configuration is chosen. A FunctionFS user space daemon may read/write to those files regardless of whether the function is enabled. If it is not, the operation will block until host enables the function. epfile->ep represents an actual USB end point, and it's number does not have to correspond to the epfile file name, and may be different in different configuration. FunctionFS hides all that from the user space daemon. epfile->ep is set when host changes configuration (i.e. function's set_alt or disable callbacks). IIRC this implies that epfile->ep cannot be protected by a mutex, and therefore is protected by a spinlock. Since it is protected by a spinlock, the ffs_epfile_io function cannot lock it and then proceed to allocating memory and copying data from user space. That's why there is the need for the loop since there is no way to guarantee that while the memory was allocated and data was read from user space (if it is a write), the function has not been disabled and re-enabled. However, I'm not sure whether maxpacketsize can change. It is part of endpoint's descriptor and even though the endpoint number can change while ffs_epfile_io is running, all the other descriptor fields should stay the same. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, Nov 05 2013, Alan Stern wrote: Maybe Michal can enlighten us. Sorry for late response, this thread fell under my radar for some reason. So here's how it works: epfile represents an end point file on the fuctionfs file system, i.e. what user space is seeing. It's numbering is independent of which USB configuration is chosen. A FunctionFS user space daemon may read/write to those files regardless of whether the function is enabled. If it is not, the operation will block until host enables the function. epfile-ep represents an actual USB end point, and it's number does not have to correspond to the epfile file name, and may be different in different configuration. FunctionFS hides all that from the user space daemon. epfile-ep is set when host changes configuration (i.e. function's set_alt or disable callbacks). IIRC this implies that epfile-ep cannot be protected by a mutex, and therefore is protected by a spinlock. Since it is protected by a spinlock, the ffs_epfile_io function cannot lock it and then proceed to allocating memory and copying data from user space. That's why there is the need for the loop since there is no way to guarantee that while the memory was allocated and data was read from user space (if it is a write), the function has not been disabled and re-enabled. However, I'm not sure whether maxpacketsize can change. It is part of endpoint's descriptor and even though the endpoint number can change while ffs_epfile_io is running, all the other descriptor fields should stay the same. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--m...@google.com--xmpp:min...@jabber.org--ooO--(_)--Ooo-- signature.asc Description: PGP signature
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, 5 Nov 2013, David Cohen wrote:
> Hi Alan,
>
> On 11/05/2013 07:38 AM, Alan Stern wrote:
> > On Tue, 5 Nov 2013, David Cohen wrote:
> >
> +/*
> + * Controller requires buffer size to be aligned to
> + * maxpacketsize of an out endpoint.
> + */
> +if (gadget->quirk_ep_out_aligned_size && read) {
> +/*
> + * We pass 'orig_len' to
> usp_ep_align_maxpacketsize()
> + * due to we're in a loop and 'len' may have
> been
> + * changed.
> + */
> +len = usb_ep_align_maxpacketsize(ep->ep,
> orig_len);
> +if (data && len > data_len) {
> +kfree(data);
> +data = NULL;
> +data_len = 0;
> +}
> +}
> >>>
> >>> Since the value of orig_len never changes, there's no point calling
> >>> usb_ep_align_maxpacketsize() inside the loop. You should call it only
> >>> once, before the loop starts. Once you do that, you won't need
> >>> orig_len at all.
> >>
> >> orig_len doesn't change but ep->ep does. If USB specs say max packet
> >> size won't change even if ep does, than we can call it from outside the
> >> loop.
> >
> > I'm not too familiar with this driver. It looks like the only way
> > ep->ep can change is if the endpoint gets enabled while you're sitting
> > inside the wait_event_interruptible() call.
> >
> > In fact, the whole structure of that loop looks peculiar. Why not
> > acquire the mutex first and then do everything else?
>
> I'm not 100% familiar with this driver too. I'd keep this change to
> another patch.
>
> >
> > Does it even make sense for ep to change? Would this change be visible
> > to the host? What if the host changes the alternate setting while this
> > loop is running -- does it make sense for the userspace program to
> > start a read or write under one altsetting but then have the read/write
> > take place under a different altsetting?
>
> It doesn't make sense to do so, but gadget driver allows it. If we just
> ignore, it would be a security or instability issue possible to xploit
> (for DWC3 and any other controller which may depend on this quirk).
Maybe Michal can enlighten us.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Hi Alan,
On 11/05/2013 07:38 AM, Alan Stern wrote:
> On Tue, 5 Nov 2013, David Cohen wrote:
>
+ /*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+ if (gadget->quirk_ep_out_aligned_size && read) {
+ /*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+ len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
+ if (data && len > data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
>>>
>>> Since the value of orig_len never changes, there's no point calling
>>> usb_ep_align_maxpacketsize() inside the loop. You should call it only
>>> once, before the loop starts. Once you do that, you won't need
>>> orig_len at all.
>>
>> orig_len doesn't change but ep->ep does. If USB specs say max packet
>> size won't change even if ep does, than we can call it from outside the
>> loop.
>
> I'm not too familiar with this driver. It looks like the only way
> ep->ep can change is if the endpoint gets enabled while you're sitting
> inside the wait_event_interruptible() call.
>
> In fact, the whole structure of that loop looks peculiar. Why not
> acquire the mutex first and then do everything else?
I'm not 100% familiar with this driver too. I'd keep this change to
another patch.
>
> Does it even make sense for ep to change? Would this change be visible
> to the host? What if the host changes the alternate setting while this
> loop is running -- does it make sense for the userspace program to
> start a read or write under one altsetting but then have the read/write
> take place under a different altsetting?
It doesn't make sense to do so, but gadget driver allows it. If we just
ignore, it would be a security or instability issue possible to xploit
(for DWC3 and any other controller which may depend on this quirk).
Br, David Cohen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, 5 Nov 2013, David Cohen wrote:
> >> + /*
> >> + * Controller requires buffer size to be aligned to
> >> + * maxpacketsize of an out endpoint.
> >> + */
> >> + if (gadget->quirk_ep_out_aligned_size && read) {
> >> + /*
> >> + * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> >> + * due to we're in a loop and 'len' may have been
> >> + * changed.
> >> + */
> >> + len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> >> + if (data && len > data_len) {
> >> + kfree(data);
> >> + data = NULL;
> >> + data_len = 0;
> >> + }
> >> + }
> >
> > Since the value of orig_len never changes, there's no point calling
> > usb_ep_align_maxpacketsize() inside the loop. You should call it only
> > once, before the loop starts. Once you do that, you won't need
> > orig_len at all.
>
> orig_len doesn't change but ep->ep does. If USB specs say max packet
> size won't change even if ep does, than we can call it from outside the
> loop.
I'm not too familiar with this driver. It looks like the only way
ep->ep can change is if the endpoint gets enabled while you're sitting
inside the wait_event_interruptible() call.
In fact, the whole structure of that loop looks peculiar. Why not
acquire the mutex first and then do everything else?
Does it even make sense for ep to change? Would this change be visible
to the host? What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On 11/05/2013 06:52 AM, Alan Stern wrote:
> On Mon, 4 Nov 2013, David Cohen wrote:
>
>> Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
>> to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
>> to pad epout buffer to match above condition if quirk is found.
>>
>> Signed-off-by: David Cohen
>> ---
>> drivers/usb/gadget/f_fs.c | 22 ++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 75e4b7846a8d..e7c3c3119552 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>> char __user *buf, size_t len, int read)
>> {
>> struct ffs_epfile *epfile = file->private_data;
>> +struct usb_gadget *gadget = epfile->ffs->gadget;
>> struct ffs_ep *ep;
>> char *data = NULL;
>> +size_t data_len = 0;
>> ssize_t ret;
>> int halt;
>> +size_t orig_len = len;
>>
>> goto first_try;
>> do {
>> @@ -794,11 +797,30 @@ first_try:
>> goto error;
>> }
>>
>> +/*
>> + * Controller requires buffer size to be aligned to
>> + * maxpacketsize of an out endpoint.
>> + */
>> +if (gadget->quirk_ep_out_aligned_size && read) {
>> +/*
>> + * We pass 'orig_len' to usp_ep_align_maxpacketsize()
>> + * due to we're in a loop and 'len' may have been
>> + * changed.
>> + */
>> +len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
>> +if (data && len > data_len) {
>> +kfree(data);
>> +data = NULL;
>> +data_len = 0;
>> +}
>> +}
>
> Since the value of orig_len never changes, there's no point calling
> usb_ep_align_maxpacketsize() inside the loop. You should call it only
> once, before the loop starts. Once you do that, you won't need
> orig_len at all.
orig_len doesn't change but ep->ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
Br, David--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On 11/05/2013 06:52 AM, Alan Stern wrote:
> On Mon, 4 Nov 2013, David Cohen wrote:
>
>> Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
>> to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
>> to pad epout buffer to match above condition if quirk is found.
>>
>> Signed-off-by: David Cohen
>> ---
>> drivers/usb/gadget/f_fs.c | 22 ++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 75e4b7846a8d..e7c3c3119552 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>> char __user *buf, size_t len, int read)
>> {
>> struct ffs_epfile *epfile = file->private_data;
>> +struct usb_gadget *gadget = epfile->ffs->gadget;
>> struct ffs_ep *ep;
>> char *data = NULL;
>> +size_t data_len = 0;
>> ssize_t ret;
>> int halt;
>> +size_t orig_len = len;
>>
>> goto first_try;
>> do {
>> @@ -794,11 +797,30 @@ first_try:
>> goto error;
>> }
>>
>> +/*
>> + * Controller requires buffer size to be aligned to
>> + * maxpacketsize of an out endpoint.
>> + */
>> +if (gadget->quirk_ep_out_aligned_size && read) {
>> +/*
>> + * We pass 'orig_len' to usp_ep_align_maxpacketsize()
>> + * due to we're in a loop and 'len' may have been
>> + * changed.
>> + */
>> +len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
>> +if (data && len > data_len) {
>> +kfree(data);
>> +data = NULL;
>> +data_len = 0;
>> +}
>> +}
>
> Since the value of orig_len never changes, there's no point calling
> usb_ep_align_maxpacketsize() inside the loop. You should call it only
> once, before the loop starts. Once you do that, you won't need
> orig_len at all.
orig_len doesn't change but ep->ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Mon, 4 Nov 2013, David Cohen wrote:
> Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
> to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
> to pad epout buffer to match above condition if quirk is found.
>
> Signed-off-by: David Cohen
> ---
> drivers/usb/gadget/f_fs.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b7846a8d..e7c3c3119552 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>char __user *buf, size_t len, int read)
> {
> struct ffs_epfile *epfile = file->private_data;
> + struct usb_gadget *gadget = epfile->ffs->gadget;
> struct ffs_ep *ep;
> char *data = NULL;
> + size_t data_len = 0;
> ssize_t ret;
> int halt;
> + size_t orig_len = len;
>
> goto first_try;
> do {
> @@ -794,11 +797,30 @@ first_try:
> goto error;
> }
>
> + /*
> + * Controller requires buffer size to be aligned to
> + * maxpacketsize of an out endpoint.
> + */
> + if (gadget->quirk_ep_out_aligned_size && read) {
> + /*
> + * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> + * due to we're in a loop and 'len' may have been
> + * changed.
> + */
> + len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> + if (data && len > data_len) {
> + kfree(data);
> + data = NULL;
> + data_len = 0;
> + }
> + }
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Mon, 4 Nov 2013, David Cohen wrote:
Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
to pad epout buffer to match above condition if quirk is found.
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/gadget/f_fs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
+ struct usb_gadget *gadget = epfile-ffs-gadget;
struct ffs_ep *ep;
char *data = NULL;
+ size_t data_len = 0;
ssize_t ret;
int halt;
+ size_t orig_len = len;
goto first_try;
do {
@@ -794,11 +797,30 @@ first_try:
goto error;
}
+ /*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+ if (gadget-quirk_ep_out_aligned_size read) {
+ /*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+ len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+ if (data len data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On 11/05/2013 06:52 AM, Alan Stern wrote:
On Mon, 4 Nov 2013, David Cohen wrote:
Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
to pad epout buffer to match above condition if quirk is found.
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/gadget/f_fs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
+struct usb_gadget *gadget = epfile-ffs-gadget;
struct ffs_ep *ep;
char *data = NULL;
+size_t data_len = 0;
ssize_t ret;
int halt;
+size_t orig_len = len;
goto first_try;
do {
@@ -794,11 +797,30 @@ first_try:
goto error;
}
+/*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+if (gadget-quirk_ep_out_aligned_size read) {
+/*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+if (data len data_len) {
+kfree(data);
+data = NULL;
+data_len = 0;
+}
+}
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
orig_len doesn't change but ep-ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On 11/05/2013 06:52 AM, Alan Stern wrote:
On Mon, 4 Nov 2013, David Cohen wrote:
Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
to pad epout buffer to match above condition if quirk is found.
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/gadget/f_fs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
+struct usb_gadget *gadget = epfile-ffs-gadget;
struct ffs_ep *ep;
char *data = NULL;
+size_t data_len = 0;
ssize_t ret;
int halt;
+size_t orig_len = len;
goto first_try;
do {
@@ -794,11 +797,30 @@ first_try:
goto error;
}
+/*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+if (gadget-quirk_ep_out_aligned_size read) {
+/*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+if (data len data_len) {
+kfree(data);
+data = NULL;
+data_len = 0;
+}
+}
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
orig_len doesn't change but ep-ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
Br, David--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, 5 Nov 2013, David Cohen wrote:
+ /*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+ if (gadget-quirk_ep_out_aligned_size read) {
+ /*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+ len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+ if (data len data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
orig_len doesn't change but ep-ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
I'm not too familiar with this driver. It looks like the only way
ep-ep can change is if the endpoint gets enabled while you're sitting
inside the wait_event_interruptible() call.
In fact, the whole structure of that loop looks peculiar. Why not
acquire the mutex first and then do everything else?
Does it even make sense for ep to change? Would this change be visible
to the host? What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Hi Alan,
On 11/05/2013 07:38 AM, Alan Stern wrote:
On Tue, 5 Nov 2013, David Cohen wrote:
+ /*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+ if (gadget-quirk_ep_out_aligned_size read) {
+ /*
+ * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have been
+ * changed.
+ */
+ len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+ if (data len data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
orig_len doesn't change but ep-ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
I'm not too familiar with this driver. It looks like the only way
ep-ep can change is if the endpoint gets enabled while you're sitting
inside the wait_event_interruptible() call.
In fact, the whole structure of that loop looks peculiar. Why not
acquire the mutex first and then do everything else?
I'm not 100% familiar with this driver too. I'd keep this change to
another patch.
Does it even make sense for ep to change? Would this change be visible
to the host? What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?
It doesn't make sense to do so, but gadget driver allows it. If we just
ignore, it would be a security or instability issue possible to xploit
(for DWC3 and any other controller which may depend on this quirk).
Br, David Cohen
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
On Tue, 5 Nov 2013, David Cohen wrote:
Hi Alan,
On 11/05/2013 07:38 AM, Alan Stern wrote:
On Tue, 5 Nov 2013, David Cohen wrote:
+/*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+if (gadget-quirk_ep_out_aligned_size read) {
+/*
+ * We pass 'orig_len' to
usp_ep_align_maxpacketsize()
+ * due to we're in a loop and 'len' may have
been
+ * changed.
+ */
+len = usb_ep_align_maxpacketsize(ep-ep,
orig_len);
+if (data len data_len) {
+kfree(data);
+data = NULL;
+data_len = 0;
+}
+}
Since the value of orig_len never changes, there's no point calling
usb_ep_align_maxpacketsize() inside the loop. You should call it only
once, before the loop starts. Once you do that, you won't need
orig_len at all.
orig_len doesn't change but ep-ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.
I'm not too familiar with this driver. It looks like the only way
ep-ep can change is if the endpoint gets enabled while you're sitting
inside the wait_event_interruptible() call.
In fact, the whole structure of that loop looks peculiar. Why not
acquire the mutex first and then do everything else?
I'm not 100% familiar with this driver too. I'd keep this change to
another patch.
Does it even make sense for ep to change? Would this change be visible
to the host? What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?
It doesn't make sense to do so, but gadget driver allows it. If we just
ignore, it would be a security or instability issue possible to xploit
(for DWC3 and any other controller which may depend on this quirk).
Maybe Michal can enlighten us.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
to pad epout buffer to match above condition if quirk is found.
Signed-off-by: David Cohen
---
drivers/usb/gadget/f_fs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file->private_data;
+ struct usb_gadget *gadget = epfile->ffs->gadget;
struct ffs_ep *ep;
char *data = NULL;
+ size_t data_len = 0;
ssize_t ret;
int halt;
+ size_t orig_len = len;
goto first_try;
do {
@@ -794,11 +797,30 @@ first_try:
goto error;
}
+ /*
+* Controller requires buffer size to be aligned to
+* maxpacketsize of an out endpoint.
+*/
+ if (gadget->quirk_ep_out_aligned_size && read) {
+ /*
+* We pass 'orig_len' to usp_ep_align_maxpacketsize()
+* due to we're in a loop and 'len' may have been
+* changed.
+*/
+ len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
+ if (data && len > data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
+
/* Allocate & copy */
if (!halt && !data) {
data = kzalloc(len, GFP_KERNEL);
if (unlikely(!data))
return -ENOMEM;
+ data_len = len;
if (!read &&
unlikely(__copy_from_user(data, buf, len))) {
--
1.8.4.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize
Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
to be aligned to maxpacketsize of an out endpoint. ffs_epfile_io() needs
to pad epout buffer to match above condition if quirk is found.
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---
drivers/usb/gadget/f_fs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file-private_data;
+ struct usb_gadget *gadget = epfile-ffs-gadget;
struct ffs_ep *ep;
char *data = NULL;
+ size_t data_len = 0;
ssize_t ret;
int halt;
+ size_t orig_len = len;
goto first_try;
do {
@@ -794,11 +797,30 @@ first_try:
goto error;
}
+ /*
+* Controller requires buffer size to be aligned to
+* maxpacketsize of an out endpoint.
+*/
+ if (gadget-quirk_ep_out_aligned_size read) {
+ /*
+* We pass 'orig_len' to usp_ep_align_maxpacketsize()
+* due to we're in a loop and 'len' may have been
+* changed.
+*/
+ len = usb_ep_align_maxpacketsize(ep-ep, orig_len);
+ if (data len data_len) {
+ kfree(data);
+ data = NULL;
+ data_len = 0;
+ }
+ }
+
/* Allocate copy */
if (!halt !data) {
data = kzalloc(len, GFP_KERNEL);
if (unlikely(!data))
return -ENOMEM;
+ data_len = len;
if (!read
unlikely(__copy_from_user(data, buf, len))) {
--
1.8.4.rc3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
