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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200601041048.GB13752@b29397-desktop>
Date:   Mon, 1 Jun 2020 04:10:24 +0000
From:   Peter Chen <peter.chen@....com>
To:     Jia-Ju Bai <baijiaju1990@...il.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "pawell@...ence.com" <pawell@...ence.com>,
        "rogerq@...com" <rogerq@...com>,
        "colin.king@...onical.com" <colin.king@...onical.com>,
        "yuehaibing@...wei.com" <yuehaibing@...wei.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: cdns3: fix possible buffer overflow caused by bad
 DMA value

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@...il.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