Fix potential shift out-of-bounds in xenhcd_hub_control()
ClearPortFeature handling and SetPortFeature handling.
wValue may be greater than 32 which can not be used for shifting.
similar patch:
https://patchwork.kernel.org/patch/12162547
Signed-off-by: Zhang Shurong <[email protected]>
---
drivers/usb/host/xen-hcd.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 46fdab940092..c0e7207d3857 100644
--- 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,
info->ports[wIndex - 1].c_connection = false;
fallthrough;
default:
+ if (wValue >= 32)
+ goto error;
info->ports[wIndex - 1].status &= ~(1 << wValue);
break;
}
@@ -527,6 +529,8 @@ static int xenhcd_hub_control(struct usb_hcd *hcd, __u16 typeReq, __u16 wValue,
xenhcd_rhport_suspend(info, wIndex);
break;
default:
+ if (wValue >= 32)
+ goto error;
if (info->ports[wIndex-1].status & USB_PORT_STAT_POWER)
info->ports[wIndex-1].status |= (1 << wValue);
}
--
2.41.0
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,
> info->ports[wIndex - 1].c_connection = false;
> fallthrough;
> default:
> + if (wValue >= 32)
> + goto error;
> info->ports[wIndex - 1].status &= ~(1 << wValue);
Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
than 1u.
Jan
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,
> > info->ports[wIndex - 1].c_connection = false;
> > fallthrough;
> > default:
> > + if (wValue >= 32)
> > + goto error;
> > info->ports[wIndex - 1].status &= ~(1 << wValue);
>
> Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> than 1u.
Why isn't the caller fixed so this type of value could never be passed
to the hub_control callback?
thanks,
greg k-h
在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> 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,> >
> > > info->ports[wIndex - 1].c_connection =
false;
> > > fallthrough;
> > >
> > > default:
> > > + if (wValue >= 32)
> > > + goto error;
> > >
> > > info->ports[wIndex - 1].status &= ~(1
<< wValue);
> >
> > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > than 1u.
>
> Why isn't the caller fixed so this type of value could never be passed
> to the hub_control callback?
>
> thanks,
>
> greg k-h
Although I'm not knowledgeable about the USB subsystem, I've observed that not
all driver code that implements hub_control callback performs a shift
operation on wValue, and not all shift operations among them cause problems.
Therefore, I've decided to fix this issue within each driver itself.
For example, in r8a66597_hub_control, it will first check whether wValue is
valid (always < 31) before the shift operation. In case of an invalid number,
the code would execute the error branch instead of the shift operation.
switch (wValue) {
case USB_PORT_FEAT_ENABLE:
rh->port &= ~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 &= ~(1 << wValue);
在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
>
> > 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,> >
> > > >
> > > > info->ports[wIndex - 1].c_connection =
>
> false;
>
> > > > fallthrough;
> > > >
> > > > default:
> > > > + if (wValue >= 32)
> > > > + goto error;
> > > >
> > > > info->ports[wIndex - 1].status &= ~(1
>
> << wValue);
>
> > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > than 1u.
> >
> > Why isn't the caller fixed so this type of value could never be passed
> > to the hub_control callback?
> >
> > thanks,
> >
> > greg k-h
>
> Although I'm not knowledgeable about the USB subsystem, I've observed that
> not all driver code that implements hub_control callback performs a shift
> operation on wValue, and not all shift operations among them cause
> problems. Therefore, I've decided to fix this issue within each driver
> itself.
>
> For example, in r8a66597_hub_control, it will first check whether wValue is
> valid (always < 31) before the shift operation. In case of an invalid
> number, the code would execute the error branch instead of the shift
> operation.
>
> switch (wValue) {
> case USB_PORT_FEAT_ENABLE:
> rh->port &= ~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 &= ~(1 << wValue);
Hi there. I apologize for reaching out once more. I'm feeling a bit puzzled
about what my next step should be. I'm unsure whether I should rewrite this
patch or attempt to address the issue at the caller level.
On Sun, Aug 06, 2023 at 10:11:43PM +0800, Zhang Shurong wrote:
> 在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> > 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> >
> > > 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,> >
> > > > >
> > > > > info->ports[wIndex - 1].c_connection =
> >
> > false;
> >
> > > > > fallthrough;
> > > > >
> > > > > default:
> > > > > + if (wValue >= 32)
> > > > > + goto error;
> > > > >
> > > > > info->ports[wIndex - 1].status &= ~(1
> >
> > << wValue);
> >
> > > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > > than 1u.
> > >
> > > Why isn't the caller fixed so this type of value could never be passed
> > > to the hub_control callback?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Although I'm not knowledgeable about the USB subsystem, I've observed that
> > not all driver code that implements hub_control callback performs a shift
> > operation on wValue, and not all shift operations among them cause
> > problems. Therefore, I've decided to fix this issue within each driver
> > itself.
> >
> > For example, in r8a66597_hub_control, it will first check whether wValue is
> > valid (always < 31) before the shift operation. In case of an invalid
> > number, the code would execute the error branch instead of the shift
> > operation.
> >
> > switch (wValue) {
> > case USB_PORT_FEAT_ENABLE:
> > rh->port &= ~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 &= ~(1 << wValue);
>
> Hi there. I apologize for reaching out once more. I'm feeling a bit puzzled
> about what my next step should be. I'm unsure whether I should rewrite this
> patch or attempt to address the issue at the caller level.
Try addressing it at the caller level first please. If that somehow
does not work, then we will take a patch series that fixes all of the
host controller drivers at once.
thanks,
greg k-h
On Sun, Aug 06, 2023 at 04:27:27PM +0200, Greg KH wrote:
> On Sun, Aug 06, 2023 at 10:11:43PM +0800, Zhang Shurong wrote:
> > 在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> > > 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> > >
> > > > 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,> >
> > > > > >
> > > > > > info->ports[wIndex - 1].c_connection =
> > >
> > > false;
> > >
> > > > > > fallthrough;
> > > > > >
> > > > > > default:
> > > > > > + if (wValue >= 32)
> > > > > > + goto error;
> > > > > >
> > > > > > info->ports[wIndex - 1].status &= ~(1
> > >
> > > << wValue);
> > >
> > > > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > > > than 1u.
> > > >
> > > > Why isn't the caller fixed so this type of value could never be passed
> > > > to the hub_control callback?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Although I'm not knowledgeable about the USB subsystem, I've observed that
> > > not all driver code that implements hub_control callback performs a shift
> > > operation on wValue, and not all shift operations among them cause
> > > problems. Therefore, I've decided to fix this issue within each driver
> > > itself.
> > >
> > > For example, in r8a66597_hub_control, it will first check whether wValue is
> > > valid (always < 31) before the shift operation. In case of an invalid
> > > number, the code would execute the error branch instead of the shift
> > > operation.
> > >
> > > switch (wValue) {
> > > case USB_PORT_FEAT_ENABLE:
> > > rh->port &= ~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 &= ~(1 << wValue);
> >
> > Hi there. I apologize for reaching out once more. I'm feeling a bit puzzled
> > about what my next step should be. I'm unsure whether I should rewrite this
> > patch or attempt to address the issue at the caller level.
>
> Try addressing it at the caller level first please. If that somehow
> does not work, then we will take a patch series that fixes all of the
> host controller drivers at once.
It's not feasible to fix all the callers, because the calls can come
from userspace via usbfs.
Alan Stern
On Tue, Aug 08, 2023 at 10:26:38AM +0200, Greg KH wrote:
> On Sun, Aug 06, 2023 at 11:15:51AM -0400, Alan Stern wrote:
> > On Sun, Aug 06, 2023 at 04:27:27PM +0200, Greg KH wrote:
> > > On Sun, Aug 06, 2023 at 10:11:43PM +0800, Zhang Shurong wrote:
> > > > 在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> > > > > 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> > > > >
> > > > > > 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,> >
> > > > > > > >
> > > > > > > > info->ports[wIndex - 1].c_connection =
> > > > >
> > > > > false;
> > > > >
> > > > > > > > fallthrough;
> > > > > > > >
> > > > > > > > default:
> > > > > > > > + if (wValue >= 32)
> > > > > > > > + goto error;
> > > > > > > >
> > > > > > > > info->ports[wIndex - 1].status &= ~(1
> > > > >
> > > > > << wValue);
> > > > >
> > > > > > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > > > > > than 1u.
> > > > > >
> > > > > > Why isn't the caller fixed so this type of value could never be passed
> > > > > > to the hub_control callback?
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Although I'm not knowledgeable about the USB subsystem, I've observed that
> > > > > not all driver code that implements hub_control callback performs a shift
> > > > > operation on wValue, and not all shift operations among them cause
> > > > > problems. Therefore, I've decided to fix this issue within each driver
> > > > > itself.
> > > > >
> > > > > For example, in r8a66597_hub_control, it will first check whether wValue is
> > > > > valid (always < 31) before the shift operation. In case of an invalid
> > > > > number, the code would execute the error branch instead of the shift
> > > > > operation.
> > > > >
> > > > > switch (wValue) {
> > > > > case USB_PORT_FEAT_ENABLE:
> > > > > rh->port &= ~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 &= ~(1 << wValue);
> > > >
> > > > Hi there. I apologize for reaching out once more. I'm feeling a bit puzzled
> > > > about what my next step should be. I'm unsure whether I should rewrite this
> > > > patch or attempt to address the issue at the caller level.
> > >
> > > Try addressing it at the caller level first please. If that somehow
> > > does not work, then we will take a patch series that fixes all of the
> > > host controller drivers at once.
> >
> > It's not feasible to fix all the callers, because the calls can come
> > from userspace via usbfs.
>
> It can? Hm, that happens through the call in rh_call_control(), right?
> But there, we do a bunch of validation before calling hub_control() so
> why can't we do the same thing in that one place as well? Making
> invalid requests from userspace should be disallowed (or we can catch
> this in the usbfs interface.)
Yes, we could filter these things out at either spot.
But that's not the best approach. The reason xen-hcd.c needs this
change in the first place is because the code is buggy, and the change
does not fix the real bug.
Section 11.24.2.2 (CLEAR PORT FEATURE) of the USB-2 spec says:
It is a Request Error if wValue is not a feature selector listed
in Table 11-17, if wIndex specifies a port that does not exist,
or if wLength is not as specified above.
xenhcd_hub_control() validates wIndex but not wValue. (In theory we
should validate wLength also, but in practice it doesn't matter.)
Here's an example from the code:
case ClearPortFeature:
if (!wIndex || wIndex > ports)
goto error;
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
xenhcd_rhport_resume(info, wIndex);
break;
case USB_PORT_FEAT_POWER:
xenhcd_rhport_power_off(info, wIndex);
break;
case USB_PORT_FEAT_ENABLE:
xenhcd_rhport_disable(info, wIndex);
break;
case USB_PORT_FEAT_C_CONNECTION:
info->ports[wIndex - 1].c_connection = false;
default:
info->ports[wIndex - 1].status &= ~(1 << wValue);
This line is wrong, and not just because wValue might be too large. The
only status bits that should be manipulated are the ones controlling
features the driver actually implements. Not random bits passed in by
the caller!
break;
}
break;
So here's what the code _should_ look like (just the end part):
case USB_PORT_FEAT_C_CONNECTION:
info->ports[wIndex - 1].c_connection = false;
fallthrough;
case USB_PORT_FEAT_C_RESET:
case USB_PORT_FEAT_C_ENABLE:
case USB_PORT_FEAT_C_SUSPEND:
info->ports[wIndex - 1].status &= ~(1 << wValue);
break;
default:
goto error;
}
break;
(Perhaps also include USB_PORT_FEAT_C_OVER_CURRENT and
USB_PORT_FEAT_INDICATOR, depending on whether the driver supports them.)
This way the driver does the right thing in all cases and it never runs
the risk of a shift amount being too big. The other HCD drivers are
written this way.
Similar reasoning applies to the SetPortFeature section.
Alan Stern
On Sun, Aug 06, 2023 at 11:15:51AM -0400, Alan Stern wrote:
> On Sun, Aug 06, 2023 at 04:27:27PM +0200, Greg KH wrote:
> > On Sun, Aug 06, 2023 at 10:11:43PM +0800, Zhang Shurong wrote:
> > > 在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> > > > 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> > > >
> > > > > 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,> >
> > > > > > >
> > > > > > > info->ports[wIndex - 1].c_connection =
> > > >
> > > > false;
> > > >
> > > > > > > fallthrough;
> > > > > > >
> > > > > > > default:
> > > > > > > + if (wValue >= 32)
> > > > > > > + goto error;
> > > > > > >
> > > > > > > info->ports[wIndex - 1].status &= ~(1
> > > >
> > > > << wValue);
> > > >
> > > > > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > > > > than 1u.
> > > > >
> > > > > Why isn't the caller fixed so this type of value could never be passed
> > > > > to the hub_control callback?
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > Although I'm not knowledgeable about the USB subsystem, I've observed that
> > > > not all driver code that implements hub_control callback performs a shift
> > > > operation on wValue, and not all shift operations among them cause
> > > > problems. Therefore, I've decided to fix this issue within each driver
> > > > itself.
> > > >
> > > > For example, in r8a66597_hub_control, it will first check whether wValue is
> > > > valid (always < 31) before the shift operation. In case of an invalid
> > > > number, the code would execute the error branch instead of the shift
> > > > operation.
> > > >
> > > > switch (wValue) {
> > > > case USB_PORT_FEAT_ENABLE:
> > > > rh->port &= ~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 &= ~(1 << wValue);
> > >
> > > Hi there. I apologize for reaching out once more. I'm feeling a bit puzzled
> > > about what my next step should be. I'm unsure whether I should rewrite this
> > > patch or attempt to address the issue at the caller level.
> >
> > Try addressing it at the caller level first please. If that somehow
> > does not work, then we will take a patch series that fixes all of the
> > host controller drivers at once.
>
> It's not feasible to fix all the callers, because the calls can come
> from userspace via usbfs.
It can? Hm, that happens through the call in rh_call_control(), right?
But there, we do a bunch of validation before calling hub_control() so
why can't we do the same thing in that one place as well? Making
invalid requests from userspace should be disallowed (or we can catch
this in the usbfs interface.)
thanks,
greg k-h