Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <tencent_62EE5604188B87B14220EA91C4CD8D4C2D06@qq.com>
From:   Zhang Shurong <zhang_shurong@foxmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     jgross@suse.com, xen-devel@lists.xenproject.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] xen: fix potential shift out-of-bounds in xenhcd_hub_control()
Date:   Sat, 01 Jul 2023 23:51:43 +0800
In-Reply-To: <2023062628-shame-ebook-56f2@gregkh>
References: <tencent_15DD79B42AD8A0D64A7CDC24D4FE6C85800A@qq.com>
 <1c8ff405-2bfe-37ff-42ba-aa4f81853475@suse.com>
 <2023062628-shame-ebook-56f2@gregkh>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

=E5=9C=A8 2023=E5=B9=B46=E6=9C=8826=E6=97=A5=E6=98=9F=E6=9C=9F=E4=B8=80 CST=
 =E4=B8=8B=E5=8D=881:52:02=EF=BC=8C=E6=82=A8=E5=86=99=E9=81=93=EF=BC=9A
> On Mon, Jun 26, 2023 at 07:48:05AM +0200, Jan Beulich wrote:
> > On 25.06.2023 18:42, Zhang Shurong wrote:
> > > --- a/drivers/usb/host/xen-hcd.c
> > > +++ b/drivers/usb/host/xen-hcd.c
> > > @@ -456,6 +456,8 @@ static int xenhcd_hub_control(struct usb_hcd *hcd,
> > > __u16 typeReq, __u16 wValue,> >=20
> > >  			info->ports[wIndex - 1].c_connection =3D=20
false;
> > >  			fallthrough;
> > >  	=09
> > >  		default:
> > > +			if (wValue >=3D 32)
> > > +				goto error;
> > >=20
> > >  			info->ports[wIndex - 1].status &=3D ~(1=20
<< wValue);
> >=20
> > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > than 1u.
>=20
> Why isn't the caller fixed so this type of value could never be passed
> to the hub_control callback?
>=20
> thanks,
>=20
> greg k-h
Although I'm not knowledgeable about the USB subsystem, I've observed that =
not=20
all driver code that implements hub_control callback performs a shift=20
operation on wValue, and not all shift operations among them cause problems=
=2E=20
Therefore, I've decided to fix this issue within each driver itself.

=46or example, in r8a66597_hub_control, it will first check whether wValue =
is=20
valid (always < 31) before the shift operation. In case of an invalid numbe=
r,=20
the code would execute the error branch instead of the shift operation.

switch (wValue) {
case USB_PORT_FEAT_ENABLE:
	rh->port &=3D ~USB_PORT_STAT_POWER;
	break;
case USB_PORT_FEAT_SUSPEND:
	break;
case USB_PORT_FEAT_POWER:
	r8a66597_port_power(r8a66597, port, 0);
	break;
case USB_PORT_FEAT_C_ENABLE:
case USB_PORT_FEAT_C_SUSPEND:
case USB_PORT_FEAT_C_CONNECTION:
case USB_PORT_FEAT_C_OVER_CURRENT:
case USB_PORT_FEAT_C_RESET:
	break;
default:
	goto error;
}
rh->port &=3D ~(1 << wValue);