Re: [PATCH] usb: cdns3: fix possible buffer overflow caused by bad DMA value

[Peter Chen <peter.chen@nxp.com>]

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 <baijiaju1990@gmail.com>
> ---
>  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