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

[Michal Nazarewicz <mina86@mina86.com>]

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 <eu@felipetonello.com>
> ---
>  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 <linux/usb/ch9.h>
>  
>  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»