* [PATCH 1/2] usb: dwc3: gadget: make Set Endpoint Configuration macros safe
@ 2017-01-31 13:08 Felipe Balbi
2017-01-31 13:08 ` [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2017-01-31 13:08 UTC (permalink / raw)
To: Linux USB; +Cc: Felipe Balbi, # v3 . 2+
Some gadget drivers are bad, bad boys. We notice
that ADB was passing bad Burst Size which caused top
bits of param0 to be overwritten which confused DWC3
when running this command.
In order to avoid future issues, we're going to make
sure values passed by macros are always safe for the
controller. Note that ADB still needs a fix to *not*
pass bad values.
Cc: <stable@vger.kernel.org> # v3.2+
Reported-by: Mohamed Abbas <mohamed.abbas@intel.com>
Sugested-by: Adam Andruszak <adam.andruszak@intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
Will be sent in a pull request during v4.11-rc
drivers/usb/dwc3/gadget.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 3129bcf74d7d..265e223ab645 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -28,23 +28,23 @@ struct dwc3;
#define gadget_to_dwc(g) (container_of(g, struct dwc3, gadget))
/* DEPCFG parameter 1 */
-#define DWC3_DEPCFG_INT_NUM(n) ((n) << 0)
+#define DWC3_DEPCFG_INT_NUM(n) (((n) & 0x1f) << 0)
#define DWC3_DEPCFG_XFER_COMPLETE_EN (1 << 8)
#define DWC3_DEPCFG_XFER_IN_PROGRESS_EN (1 << 9)
#define DWC3_DEPCFG_XFER_NOT_READY_EN (1 << 10)
#define DWC3_DEPCFG_FIFO_ERROR_EN (1 << 11)
#define DWC3_DEPCFG_STREAM_EVENT_EN (1 << 13)
-#define DWC3_DEPCFG_BINTERVAL_M1(n) ((n) << 16)
+#define DWC3_DEPCFG_BINTERVAL_M1(n) (((n) & 0xff) << 16)
#define DWC3_DEPCFG_STREAM_CAPABLE (1 << 24)
-#define DWC3_DEPCFG_EP_NUMBER(n) ((n) << 25)
+#define DWC3_DEPCFG_EP_NUMBER(n) (((n) & 0x1f) << 25)
#define DWC3_DEPCFG_BULK_BASED (1 << 30)
#define DWC3_DEPCFG_FIFO_BASED (1 << 31)
/* DEPCFG parameter 0 */
-#define DWC3_DEPCFG_EP_TYPE(n) ((n) << 1)
-#define DWC3_DEPCFG_MAX_PACKET_SIZE(n) ((n) << 3)
-#define DWC3_DEPCFG_FIFO_NUMBER(n) ((n) << 17)
-#define DWC3_DEPCFG_BURST_SIZE(n) ((n) << 22)
+#define DWC3_DEPCFG_EP_TYPE(n) (((n) & 0x3) << 1)
+#define DWC3_DEPCFG_MAX_PACKET_SIZE(n) (((n) & 0x7ff) << 3)
+#define DWC3_DEPCFG_FIFO_NUMBER(n) (((n) & 0x1f) << 17)
+#define DWC3_DEPCFG_BURST_SIZE(n) (((n) & 0xf) << 22)
#define DWC3_DEPCFG_DATA_SEQ_NUM(n) ((n) << 26)
/* This applies for core versions earlier than 1.94a */
#define DWC3_DEPCFG_IGN_SEQ_NUM (1 << 31)
--
2.11.0.295.gd7dffce1ce
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along
2017-01-31 13:08 [PATCH 1/2] usb: dwc3: gadget: make Set Endpoint Configuration macros safe Felipe Balbi
@ 2017-01-31 13:08 ` Felipe Balbi
2017-01-31 15:20 ` Krzysztof Opasiak
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2017-01-31 13:08 UTC (permalink / raw)
To: Linux USB; +Cc: Felipe Balbi, stable
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;
+
ret = usb_ep_enable(ep->ep);
if (likely(!ret)) {
epfile->ep = ep;
--
2.11.0.295.gd7dffce1ce
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along
2017-01-31 13:08 ` [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along Felipe Balbi
@ 2017-01-31 15:20 ` Krzysztof Opasiak
2017-01-31 15:56 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Opasiak @ 2017-01-31 15:20 UTC (permalink / raw)
To: Felipe Balbi, Linux USB; +Cc: stable
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?
Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along
2017-01-31 15:20 ` Krzysztof Opasiak
@ 2017-01-31 15:56 ` Felipe Balbi
2017-01-31 16:40 ` Krzysztof Opasiak
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2017-01-31 15:56 UTC (permalink / raw)
To: Krzysztof Opasiak, Linux USB; +Cc: stable
Hi,
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?
--
balbi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along
2017-01-31 15:56 ` Felipe Balbi
@ 2017-01-31 16:40 ` Krzysztof Opasiak
2017-03-10 11:43 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Opasiak @ 2017-01-31 16:40 UTC (permalink / raw)
To: Felipe Balbi, Linux USB; +Cc: stable
On 01/31/2017 04:56 PM, Felipe Balbi wrote:
>
> Hi,
>
> 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.
Cheers,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along
2017-01-31 16:40 ` Krzysztof Opasiak
@ 2017-03-10 11:43 ` Felipe Balbi
0 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2017-03-10 11:43 UTC (permalink / raw)
To: Krzysztof Opasiak, Linux USB; +Cc: stable
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-10 11:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-31 13:08 [PATCH 1/2] usb: dwc3: gadget: make Set Endpoint Configuration macros safe Felipe Balbi
2017-01-31 13:08 ` [PATCH 2/2] usb: gadget: function: f_fs: pass companion descriptor along Felipe Balbi
2017-01-31 15:20 ` Krzysztof Opasiak
2017-01-31 15:56 ` Felipe Balbi
2017-01-31 16:40 ` Krzysztof Opasiak
2017-03-10 11:43 ` Felipe Balbi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).