Re: [PATCH] USB: Always select config with the highest supported UAC version

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Mon, Feb 12, 2024 at 06:28:48PM +0300, Alexander Tsoy wrote:
> Config with the highest supported UAC version is always preferable because
> it usually provides more features.
> 
> Main motivation for this change is to improve support for several UAC2
> devices which have UAC1 config as the first one for compatibility reasons.
> UAC2 mode provides a wider range of supported sampling rates on these
> devices. Also when UAC4 support is implemented, it will need just one
> additional case line without changing the logic.
> 
> Signed-off-by: Alexander Tsoy <alexander@xxxxxxx>
> ---
>  drivers/usb/core/generic.c | 39 +++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index b134bff5c3fe..8aeb180e1cf9 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -48,9 +48,16 @@ static bool is_audio(struct usb_interface_descriptor *desc)
>  	return desc->bInterfaceClass == USB_CLASS_AUDIO;
>  }
>  
> -static bool is_uac3_config(struct usb_interface_descriptor *desc)
> +static bool is_supported_uac(struct usb_interface_descriptor *desc)
>  {
> -	return desc->bInterfaceProtocol == UAC_VERSION_3;
> +	switch(desc->bInterfaceProtocol) {
> +	case UAC_VERSION_1:
> +	case UAC_VERSION_2:
> +	case UAC_VERSION_3:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }
>  
>  int usb_choose_configuration(struct usb_device *udev)
> @@ -135,22 +142,28 @@ int usb_choose_configuration(struct usb_device *udev)
>  		}
>  
>  		/*
> -		 * Select first configuration as default for audio so that
> -		 * devices that don't comply with UAC3 protocol are supported.
> -		 * But, still iterate through other configurations and
> -		 * select UAC3 compliant config if present.
> +		 * Iterate through audio configurations and select the first of
> +		 * the highest supported UAC versions. Select the first config
> +		 * if no supported UAC configs are found.
>  		 */
>  		if (desc && is_audio(desc)) {
> -			/* Always prefer the first found UAC3 config */
> -			if (is_uac3_config(desc)) {
> -				best = c;
> -				break;
> -			}
> +			struct usb_interface_descriptor	*best_desc = NULL;
> +
> +			if (best)
> +				best_desc = &best->intf_cache[0]->altsetting->desc;

Are you sure you always have a intf_cache[0] pointer?  What about
altsetting?  Remember, invalid descriptors happen all the time, and are
a known "attack vector" against the USB stack.

>  
> -			/* If there is no UAC3 config, prefer the first config */
> -			else if (i == 0)
> +			if (i == 0)
>  				best = c;
>  
> +			/* Assume that bInterfaceProtocol value is always
> +			 * growing when UAC versions are incremented, so that
> +			 * the direct comparison is possible. */

How do we know this assumption is always true?  What happens when it is not?

> +			else if (is_supported_uac(desc) && best_desc &&
> +				 (!is_supported_uac(best_desc) ||
> +				  (desc->bInterfaceProtocol >
> +				   best_desc->bInterfaceProtocol)))
> +					best = c;

I really can't understand this if logic, sorry, can you describe it
better so that we can maintain it over time?

thanks,

greg k-h