On 20-07-01 09:52:43, Felipe Balbi wrote:
> Peter Chen <peter.chen@xxxxxxx> 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
