Re: [PATCH v3] USB: ehci_brcm_hub_control: improve port index sanitizing

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, Nov 12, 2021 at 05:49:21PM +0800, Haimin Zhang wrote:
> Due to (wIndex & 0xff) - 1 can get an integer greater than 0xf, this
> can cause array index to be out of bounds since the size of array
> port_status is 0xf. Using macro function HCS_N_PORTS() can return
> a valid index less than 15. Macro function HCS_N_PORTS() is used
> to obtain a valid port index by logical AND 0xf.
> 
> Reported-by: TCS Robot <tcs_robot@tencent.com>
> Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
> ---
>  drivers/usb/host/ehci-brcm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> index d3626bfa966b..d2bf7768349c 100644
> --- a/drivers/usb/host/ehci-brcm.c
> +++ b/drivers/usb/host/ehci-brcm.c
> @@ -62,8 +62,11 @@ static int ehci_brcm_hub_control(
>  	u32 __iomem	*status_reg;
>  	unsigned long flags;
>  	int retval, irq_disabled = 0;
> +	u32 temp;
>  
> -	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
> +	temp = (wIndex & 0xff) - 1;
> +	temp = HCS_N_PORTS(temp);

This is a misuse of the HCS_N_PORTS macro; the fact that it works out 
okay is just a coincidence.  That macro was intended for extracting the 
number of ports from the hcs_params word in the EHCI register space.  It 
should not be used for any other purpose (including forcing a value to 
be in the range of valid port numbers).

If you want to do this correctly, do something like:

	temp = min_t(u32, temp, HCS_N_PORTS_MAX - 1);

Or even:

	if (temp >= HCS_N_PORTS_MAX)	/* Force valid port number */
		temp = 0;

Alan Stern

> +	status_reg = &ehci->regs->port_status[temp];
>  
>  	/*
>  	 * RESUME is cleared when GetPortStatus() is called 20ms after start
> -- 
> 2.30.1 (Apple Git-130)