Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Peter Chen <peter.chen@nxp.com>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "pawell@cadence.com" <pawell@cadence.com>,
        "rogerq@ti.com" <rogerq@ti.com>,
        "colin.king@canonical.com" <colin.king@canonical.com>,
        "yuehaibing@huawei.com" <yuehaibing@huawei.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: cdns3: fix possible buffer overflow caused by bad
 DMA value
Thread-Topic: [PATCH] usb: cdns3: fix possible buffer overflow caused by bad
 DMA value
Thread-Index: AQHWNjHPz6qvJgpZ+karMYRZFy9ss6jDKN4A
Date:   Mon, 1 Jun 2020 04:10:24 +0000
Message-ID: <20200601041048.GB13752@b29397-desktop>
References: <20200530032400.12743-1-baijiaju1990@gmail.com>
In-Reply-To: <20200530032400.12743-1-baijiaju1990@gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8DFBDA757AA7E047A20991C996909877@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 28dd596c-3efd-49fb-026b-08d805e1b592
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jun 2020 04:10:24.2959
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: UT6gjy/IQETs6N5KOPBsIiYgrw056Eq/YEV1Xjq43k0ila94ietYwMdsx0dhMOzZWug8OBBQFm93Ho5I/ITLhw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB7096
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 20-05-30 11:24:00, Jia-Ju Bai wrote:
> In cdns3_ep0_setup_phase():
>   struct usb_ctrlrequest *ctrl =3D priv_dev->setup_buf;
>=20
> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored=20
> in DMA memory, and thus ctrl is a DMA value.
>=20
> cdns3_ep0_setup_phase()
>   cdns3_ep0_standard_request(priv_dev, ctrl)
>     cdns3_req_ep0_get_status(priv_dev, ctrl)
>       index =3D cdns3_ep_addr_to_index(ctrl->wIndex);
>       priv_ep =3D priv_dev->eps[index];
>=20
> 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 =3D cdns3_ep_addr_to_index(ctrl->wIndex);
>         priv_ep =3D priv_dev->eps[index];
>=20
> 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.
>=20

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

>=20
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/usb/cdns3/ep0.c | 5 +++++
>  1 file changed, 5 insertions(+)
>=20
> 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_devi=
ce *priv_dev,
>  		return cdns3_ep0_delegate_req(priv_dev, ctrl);
>  	case USB_RECIP_ENDPOINT:
>  		index =3D cdns3_ep_addr_to_index(ctrl->wIndex);
> +		if (index >=3D CDNS3_ENDPOINTS_MAX_COUNT)
> +			return -EINVAL;
>  		priv_ep =3D priv_dev->eps[index];
> =20
>  		/* check if endpoint is stalled or stall is pending */
> @@ -388,6 +390,9 @@ static int cdns3_ep0_feature_handle_endpoint(struct c=
dns3_device *priv_dev,
>  		return 0;
> =20
>  	index =3D cdns3_ep_addr_to_index(ctrl->wIndex);
> +	if (index >=3D CDNS3_ENDPOINTS_MAX_COUNT)
> +		return -EINVAL;
> +
>  	priv_ep =3D priv_dev->eps[index];
> =20
>  	cdns3_select_ep(priv_dev, ctrl->wIndex);
> --=20
> 2.17.1
>=20

--=20

Thanks,
Peter Chen=