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:     Felipe Balbi <balbi@kernel.org>
CC:     Jia-Ju Bai <baijiaju1990@gmail.com>,
        "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
Thread-Topic: [PATCH] usb: cdns3: fix possible buffer overflow caused by bad
 DMA value
Thread-Index: AQHWNjHPz6qvJgpZ+karMYRZFy9ss6jDKN4AgC9TLoCAABvOAA==
Date:   Wed, 1 Jul 2020 08:31:51 +0000
Message-ID: <20200701083214.GA22478@b29397-desktop>
References: <20200530032400.12743-1-baijiaju1990@gmail.com>
 <20200601041048.GB13752@b29397-desktop> <87366b916s.fsf@kernel.org>
In-Reply-To: <87366b916s.fsf@kernel.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <06751ABC84F89F4EA306AC8035D399D0@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0326c165-9d79-4618-59db-08d81d993413
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jul 2020 08:31:51.2745
 (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: YZNOSTJ/BoefWj9euf8mSaxOXNFyOQnEFD4B7hTzMD9dyxOuL1SDvuHLharemZog0SnmjC4jEwDMm9YsyWGo0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB4856
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-07-01 09:52:43, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
>=20
> > 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 store=
d=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 maliciou=
s
> >> 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 i=
s
> > 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.
>=20
> 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

>=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_d=
evice *priv_dev,
> >>  		return cdns3_ep0_delegate_req(priv_dev, ctrl);
> >>  	case USB_RECIP_ENDPOINT:
> >>  		index =3D cdns3_ep_addr_to_index(ctrl->wIndex);
>=20
> 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 m=
ask)
>   */
>  u8 cdns3_ep_addr_to_index(u8 ep_addr)
>  {
> -       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,
>=20
> This will guarantee that the number is never over the limit.
>=20
> --=20
> balbi


--=20

Thanks,
Peter Chen=