In cdns3_ep0_setup_phase():
struct usb_ctrlrequest *ctrl = priv_dev->setup_buf;
Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored
in DMA memory, and thus ctrl is a DMA value.
cdns3_ep0_setup_phase()
cdns3_ep0_standard_request(priv_dev, ctrl)
cdns3_req_ep0_get_status(priv_dev, ctrl)
index = cdns3_ep_addr_to_index(ctrl->wIndex);
priv_ep = priv_dev->eps[index];
cdns3_ep0_setup_phase()
cdns3_ep0_standard_request(priv_dev, ctrl)
cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0)
cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set)
index = cdns3_ep_addr_to_index(ctrl->wIndex);
priv_ep = priv_dev->eps[index];
In these cases, ctrl->wIndex can be be modified at anytime by malicious
hardware, and thus a buffer overflow can occur when the code
"priv_dev->eps[index]" is executed.
To fix these possible bugs, index is checked before being used.
Signed-off-by: Jia-Ju Bai <[email protected]>
---
drivers/usb/cdns3/ep0.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
index e71240b386b4..0a80c7ade613 100644
--- a/drivers/usb/cdns3/ep0.c
+++ b/drivers/usb/cdns3/ep0.c
@@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
return cdns3_ep0_delegate_req(priv_dev, ctrl);
case USB_RECIP_ENDPOINT:
index = cdns3_ep_addr_to_index(ctrl->wIndex);
+ if (index >= CDNS3_ENDPOINTS_MAX_COUNT)
+ return -EINVAL;
priv_ep = priv_dev->eps[index];
/* check if endpoint is stalled or stall is pending */
@@ -388,6 +390,9 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev,
return 0;
index = cdns3_ep_addr_to_index(ctrl->wIndex);
+ if (index >= CDNS3_ENDPOINTS_MAX_COUNT)
+ return -EINVAL;
+
priv_ep = priv_dev->eps[index];
cdns3_select_ep(priv_dev, ctrl->wIndex);
--
2.17.1
> To fix these possible bugs, index is checked before being used.
How do you think about a wording variant like the following?
Thus check the index before using it further.
Would you like to add the tag “Fixes” to the commit message?
Regards,
Markus
On 20-05-30 11:24:00, Jia-Ju Bai wrote:
> In cdns3_ep0_setup_phase():
> struct usb_ctrlrequest *ctrl = priv_dev->setup_buf;
>
> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored
> in DMA memory, and thus ctrl is a DMA value.
>
> cdns3_ep0_setup_phase()
> cdns3_ep0_standard_request(priv_dev, ctrl)
> cdns3_req_ep0_get_status(priv_dev, ctrl)
> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> priv_ep = priv_dev->eps[index];
>
> cdns3_ep0_setup_phase()
> cdns3_ep0_standard_request(priv_dev, ctrl)
> cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0)
> cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set)
> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> priv_ep = priv_dev->eps[index];
>
> In these cases, ctrl->wIndex can be be modified at anytime by malicious
> hardware, and thus a buffer overflow can occur when the code
> "priv_dev->eps[index]" is executed.
>
Did you see the setup buffer is overwritten before the setup handling is
finished?
> To fix these possible bugs, index is checked before being used.
I think the better fix is to use one additional buffer for struct
usb_ctrlrequest, and copy the dma_buf to it after setup packet
has received, then use the value stored in this buffer for later
operation, it could avoid quitting the code which is useful in fact.
Peter
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/usb/cdns3/ep0.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
> index e71240b386b4..0a80c7ade613 100644
> --- a/drivers/usb/cdns3/ep0.c
> +++ b/drivers/usb/cdns3/ep0.c
> @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
> return cdns3_ep0_delegate_req(priv_dev, ctrl);
> case USB_RECIP_ENDPOINT:
> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> + if (index >= CDNS3_ENDPOINTS_MAX_COUNT)
> + return -EINVAL;
> priv_ep = priv_dev->eps[index];
>
> /* check if endpoint is stalled or stall is pending */
> @@ -388,6 +390,9 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev,
> return 0;
>
> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> + if (index >= CDNS3_ENDPOINTS_MAX_COUNT)
> + return -EINVAL;
> +
> priv_ep = priv_dev->eps[index];
>
> cdns3_select_ep(priv_dev, ctrl->wIndex);
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Peter Chen <[email protected]> writes:
> On 20-05-30 11:24:00, Jia-Ju Bai wrote:
>> In cdns3_ep0_setup_phase():
>> struct usb_ctrlrequest *ctrl = priv_dev->setup_buf;
>>
>> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored
>> in DMA memory, and thus ctrl is a DMA value.
>>
>> cdns3_ep0_setup_phase()
>> cdns3_ep0_standard_request(priv_dev, ctrl)
>> cdns3_req_ep0_get_status(priv_dev, ctrl)
>> index = cdns3_ep_addr_to_index(ctrl->wIndex);
>> priv_ep = priv_dev->eps[index];
>>
>> cdns3_ep0_setup_phase()
>> cdns3_ep0_standard_request(priv_dev, ctrl)
>> cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0)
>> cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set)
>> index = cdns3_ep_addr_to_index(ctrl->wIndex);
>> priv_ep = priv_dev->eps[index];
>>
>> In these cases, ctrl->wIndex can be be modified at anytime by malicious
>> hardware, and thus a buffer overflow can occur when the code
>> "priv_dev->eps[index]" is executed.
>>
>
> Did you see the setup buffer is overwritten before the setup handling is
> finished?
>
>> To fix these possible bugs, index is checked before being used.
>
> I think the better fix is to use one additional buffer for struct
> usb_ctrlrequest, and copy the dma_buf to it after setup packet
> has received, then use the value stored in this buffer for later
> operation, it could avoid quitting the code which is useful in fact.
Why is this a better fix? If you don't have that endpoint index, you
shouldn't try to access it. However, I think the problem here is
slightly easier to solve :-)
>> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>> index e71240b386b4..0a80c7ade613 100644
>> --- a/drivers/usb/cdns3/ep0.c
>> +++ b/drivers/usb/cdns3/ep0.c
>> @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
>> return cdns3_ep0_delegate_req(priv_dev, ctrl);
>> case USB_RECIP_ENDPOINT:
>> index = cdns3_ep_addr_to_index(ctrl->wIndex);
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 5e24c2e57c0d..96ba3eec805c 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -107,7 +107,10 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
*/
u8 cdns3_ep_addr_to_index(u8 ep_addr)
{
- return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0));
+ u8 num = ep_addr & USB_ENDPOINT_NUMBER_MASK;
+ u8 dir = ep_addr & USB_ENDPOINT_DIR_MASK;
+
+ return num + dir ? 16 : 0;
}
static int cdns3_get_dma_pos(struct cdns3_device *priv_dev,
This will guarantee that the number is never over the limit.
--
balbi
On 20-07-01 09:52:43, Felipe Balbi wrote:
> Peter Chen <[email protected]> writes:
>
> > On 20-05-30 11:24:00, Jia-Ju Bai wrote:
> >> In cdns3_ep0_setup_phase():
> >> struct usb_ctrlrequest *ctrl = priv_dev->setup_buf;
> >>
> >> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored
> >> in DMA memory, and thus ctrl is a DMA value.
> >>
> >> cdns3_ep0_setup_phase()
> >> cdns3_ep0_standard_request(priv_dev, ctrl)
> >> cdns3_req_ep0_get_status(priv_dev, ctrl)
> >> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> >> priv_ep = priv_dev->eps[index];
> >>
> >> cdns3_ep0_setup_phase()
> >> cdns3_ep0_standard_request(priv_dev, ctrl)
> >> cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0)
> >> cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set)
> >> index = cdns3_ep_addr_to_index(ctrl->wIndex);
> >> priv_ep = priv_dev->eps[index];
> >>
> >> In these cases, ctrl->wIndex can be be modified at anytime by malicious
> >> hardware, and thus a buffer overflow can occur when the code
> >> "priv_dev->eps[index]" is executed.
> >>
> >
> > Did you see the setup buffer is overwritten before the setup handling is
> > finished?
> >
> >> To fix these possible bugs, index is checked before being used.
> >
> > I think the better fix is to use one additional buffer for struct
> > usb_ctrlrequest, and copy the dma_buf to it after setup packet
> > has received, then use the value stored in this buffer for later
> > operation, it could avoid quitting the code which is useful in fact.
>
> Why is this a better fix? If you don't have that endpoint index, you
> shouldn't try to access it. However, I think the problem here is
> slightly easier to solve :-)
The possible problem here is: it is a correct setup packet, the memory
it uses may be modified by controller wrongly (eg, try to get next setup
packet) before it finishes using. So, I suggest adding a setup buf for
struct cdns3 to store every setup packet after it receives to avoid
the original setup buffer corrupted.
Peter
>
> >> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
> >> index e71240b386b4..0a80c7ade613 100644
> >> --- a/drivers/usb/cdns3/ep0.c
> >> +++ b/drivers/usb/cdns3/ep0.c
> >> @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev,
> >> return cdns3_ep0_delegate_req(priv_dev, ctrl);
> >> case USB_RECIP_ENDPOINT:
> >> index = cdns3_ep_addr_to_index(ctrl->wIndex);
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 5e24c2e57c0d..96ba3eec805c 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -107,7 +107,10 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
> */
> u8 cdns3_ep_addr_to_index(u8 ep_addr)
> {
> - return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0));
> + u8 num = ep_addr & USB_ENDPOINT_NUMBER_MASK;
> + u8 dir = ep_addr & USB_ENDPOINT_DIR_MASK;
> +
> + return num + dir ? 16 : 0;
> }
>
> static int cdns3_get_dma_pos(struct cdns3_device *priv_dev,
>
> This will guarantee that the number is never over the limit.
>
> --
> balbi
--
Thanks,
Peter Chen