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:   Felipe Balbi <balbi@kernel.org>
To:     Peter Chen <peter.chen@nxp.com>,
        Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     "gregkh\@linuxfoundation.org" <gregkh@linuxfoundation.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
In-Reply-To: <20200601041048.GB13752@b29397-desktop>
References: <20200530032400.12743-1-baijiaju1990@gmail.com> <20200601041048.GB13752@b29397-desktop>
Date:   Wed, 01 Jul 2020 09:52:43 +0300
Message-ID: <87366b916s.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

Peter Chen <peter.chen@nxp.com> writes:

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

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_dev=
ice *priv_dev,
>>  		return cdns3_ep0_delegate_req(priv_dev, ctrl);
>>  	case USB_RECIP_ENDPOINT:
>>  		index =3D 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
=2D-- 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 mas=
k)
  */
 u8 cdns3_ep_addr_to_index(u8 ep_addr)
 {
=2D       return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0));
+       u8 num =3D ep_addr & USB_ENDPOINT_NUMBER_MASK;
+       u8 dir =3D ep_addr & USB_ENDPOINT_DIR_MASK;
+
+       return num + dir ? 16 : 0;
 }
=20
 static int cdns3_get_dma_pos(struct cdns3_device *priv_dev,

This will guarantee that the number is never over the limit.

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAl78MrsACgkQzL64meEa
mQZjSxAAjXPp6UT5McnCzjDtput+cJ0BrzbOr9wmnVmU10pd6VKGxFzuljOOdQTQ
LrXE6JPxgvBX+iS1i+6WEsbQrheEIKWht3lVzNuufZ5sFBsmXjU+Xv6bspsdctkv
llWLp95AZ3GVB0x7TQfeyqL1qGVZpuCU8dkI2LIeLdW9amT/WFE4af0kxp2bOzU0
5/mTAmXgv/cn5AjZj9WxljvedNwyr9dqTVxZt1rGzgLgSz1l5Ab3KnZqh6m5FJ5N
sXAQttuo+Yod0NP7SRNuZDBMyPE1f4cdak/j34qnHMP1WrFh8DJPk67xHd88+pj6
ubeJY93IdmX/uYov85v84Z26GnEI5gK+UVmmiO5HSyR5q72dI5L6sxMwzeUv8djQ
QvtMxLJqT4/JM6qUvYjIfK/OPTPPkzQRv+Jn93JfIavjdlNMkku+jT+8tADMWoyX
jCSmyIRssUXhotr2ZoiTbD32sqAYRIu2henWU6t9dsUus3Kp7o90daJ+bZsl3l4z
AKEYjXmgQDI34B4TR9vvtSPldKujG0bEm6XggwZXlxtuWMyTMobnWvUum71adDul
ZYOy6HnFJbZwbejM8/ekWeFLq1+8MPddlI2rlZS76SdCCXgSPvqCgCgtndSTkYVM
QDSxXd+WhUEoSVZUTWJ+tL7Yl3qvSBz7LOLfxz4Vz3Z9J+Gp+PI=
=K1QB
-----END PGP SIGNATURE-----
--=-=-=--