linux-kernel - Re: [PATCH] xen: fix potential shift out-of-bounds in xenhcd_hub

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <tencent_942CC5C35E410E3545C2E386BE566B8B1405@qq.com>
Date:   Sun, 06 Aug 2023 22:11:43 +0800
From:   Zhang Shurong <zhang_shurong@...mail.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     jgross@...e.com, xen-devel@...ts.xenproject.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xen: fix potential shift out-of-bounds in xenhcd_hub_control()

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