Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 20:59, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
>> always aligned with wMaxPacketSize (512 usually). This makes sure
>> that no buffer has the wrong size, which can cause nasty bugs.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/u_f.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>> index 4bc7eea8bfc8..d1933b0b76c3 100644
>> --- a/drivers/usb/gadget/u_f.c
>> +++ b/drivers/usb/gadget/u_f.c
>> @@ -12,6 +12,7 @@
>>   */
>>  
>>  #include "u_f.h"
>> +#include 
>>  
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
>> default_len)
>>  {
>> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 
>> len, int default_len)
>>  req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>  if (req) {
>>  req->length = len ?: default_len;
>> +if (usb_endpoint_dir_out(ep->desc))
>> +req->length = usb_ep_align(ep, req->length);
>>  req->buf = kmalloc(req->length, GFP_ATOMIC);
>>  if (!req->buf) {
>>  usb_ep_free_request(ep, req);
> 
> I’m a bit scared of this change.

I agree, it's scary. :D

> 
> Drivers which call alloc_ep_req and then ignore req->length using the
> same length they passed to the function will silently drop data.
> 
> Drivers which do not ignore req->length may end up overwriting some
> other buffer, e.g.:
> 
>   some_buffer = kmalloc(length, GFP_KERNEL);
> req = alloc_ep_req(ep, length, 0);
> … later …
> memcpy(some_buffer, req->buf, req->length);

True. The same happens if the data associated with an OUT endpoint is
smaller than wMaxPacketSize.

This patch doesn't fix all problems associated with that, but it allows
better practice to take place. It returns to the driver the actual
allocated size, like several POSIX functions.

I haven't seen any problems on all gadgets that rely on alloc_ep_req().
Maybe as we port other gadgets to this use this function instead of
usb_ep_alloc_request() we might find some issues.

Perhaps we should add better documentation to alloc_ep_req()?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
> always aligned with wMaxPacketSize (512 usually). This makes sure
> that no buffer has the wrong size, which can cause nasty bugs.
>
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/u_f.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
> index 4bc7eea8bfc8..d1933b0b76c3 100644
> --- a/drivers/usb/gadget/u_f.c
> +++ b/drivers/usb/gadget/u_f.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "u_f.h"
> +#include 
>  
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>  {
> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 
> len, int default_len)
>   req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>   if (req) {
>   req->length = len ?: default_len;
> + if (usb_endpoint_dir_out(ep->desc))
> + req->length = usb_ep_align(ep, req->length);
>   req->buf = kmalloc(req->length, GFP_ATOMIC);
>   if (!req->buf) {
>   usb_ep_free_request(ep, req);

I’m a bit scared of this change.

Drivers which call alloc_ep_req and then ignore req->length using the
same length they passed to the function will silently drop data.

Drivers which do not ignore req->length may end up overwriting some
other buffer, e.g.:

some_buffer = kmalloc(length, GFP_KERNEL);
req = alloc_ep_req(ep, length, 0);
… later …
memcpy(some_buffer, req->buf, req->length);

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

[PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe F. Tonello]

Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
always aligned with wMaxPacketSize (512 usually). This makes sure
that no buffer has the wrong size, which can cause nasty bugs.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/u_f.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 4bc7eea8bfc8..d1933b0b76c3 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -12,6 +12,7 @@
  */
 
 #include "u_f.h"
+#include 
 
 struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
 {
@@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, 
int default_len)
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req) {
req->length = len ?: default_len;
+   if (usb_endpoint_dir_out(ep->desc))
+   req->length = usb_ep_align(ep, req->length);
req->buf = kmalloc(req->length, GFP_ATOMIC);
if (!req->buf) {
usb_ep_free_request(ep, req);
-- 
2.9.0