Re: [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along

[Felipe Balbi <felipe.balbi@linux.intel.com>]

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


Hi,

Krzysztof Opasiak <k.opasiak@samsung.com> writes:
>> Krzysztof Opasiak <k.opasiak@samsung.com> writes:
>>> On 01/31/2017 02:08 PM, Felipe Balbi wrote:
>>>> If we're dealing with SuperSpeed endpoints, we need
>>>> to make sure to pass along the companion descriptor
>>>> and initialize fields needed by the Gadget
>>>> API. Eventually, f_fs.c should be converted to use
>>>> config_ep_by_speed() like all other functions,
>>>> though.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>>>> ---
>>>>
>>>> Will be sent in a pull request during v4.11-rc
>>>>
>>>>  drivers/usb/gadget/function/f_fs.c | 15 +++++++++++++--
>>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>> index 87fccf611b69..86aba2ebb3ef 100644
>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>> @@ -1833,11 +1833,14 @@ static int ffs_func_eps_enable(struct ffs_function *func)
>>>>  	spin_lock_irqsave(&func->ffs->eps_lock, flags);
>>>>  	while(count--) {
>>>>  		struct usb_endpoint_descriptor *ds;
>>>> +		struct usb_ss_ep_comp_descriptor *comp_desc = NULL;
>>>> +		int needs_comp_desc = false;
>>>>  		int desc_idx;
>>>>  
>>>> -		if (ffs->gadget->speed == USB_SPEED_SUPER)
>>>> +		if (ffs->gadget->speed == USB_SPEED_SUPER) {
>>>>  			desc_idx = 2;
>>>> -		else if (ffs->gadget->speed == USB_SPEED_HIGH)
>>>> +			needs_comp_desc = true;
>>>> +		} else if (ffs->gadget->speed == USB_SPEED_HIGH)
>>>>  			desc_idx = 1;
>>>>  		else
>>>>  			desc_idx = 0;
>>>> @@ -1854,6 +1857,14 @@ static int ffs_func_eps_enable(struct ffs_function *func)
>>>>  
>>>>  		ep->ep->driver_data = ep;
>>>>  		ep->ep->desc = ds;
>>>> +
>>>> +		comp_desc = (struct usb_ss_ep_comp_descriptor *)(ds +
>>>> +				USB_DT_ENDPOINT_SIZE);
>>>> +		ep->ep->maxburst = comp_desc->bMaxBurst + 1;
>>>> +
>>>> +		if (needs_comp_desc)
>>>> +			ep->ep->comp_desc = comp_desc;
>>>> +
>>>
>>> Please correct me if I'm wrong but wouldn't we read rubbish here if user
>>> provided us SS ep descriptor without companion descriptor?
>> 
>> companion desc is required for SS endpoints, it's also required that
>> they follow EP desc. If user doesn't write it, don't they deserve the
>> errors they'll have?
>> 
>
> But do we deserve to access potentially unallocated memory inside kernel
> each time when some malicious application requests this?;)
>
> In my humble opinion user should get -EINVAL or sth like this from
> write(desc, sizeof(desc)) instead of some random data in companion
> descriptor.

We are *already* getting random data in companion descriptor :-) But I
agree that -EINVAL would be really nice. Can you cook up a patch for
that?

-- 
balbi

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