[PATCH] USB: Always select config with the highest supported UAC version
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@tsoy.me --- 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;
- /* 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. */ + else if (is_supported_uac(desc) && best_desc && + (!is_supported_uac(best_desc) || + (desc->bInterfaceProtocol > + best_desc->bInterfaceProtocol))) + best = c; + /* Unconditional continue, because the rest of the code * in the loop is irrelevant for audio devices, and * because it can reassign best, which for audio devices
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@tsoy.me
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.
/* 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
On Tue, 13 Feb 2024 12:05:47 +0100, Greg Kroah-Hartman wrote:
On Mon, Feb 12, 2024 at 06:28:48PM +0300, Alexander Tsoy wrote:
/* 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?
I believe this assumption is acceptable. It's all about the protocol number from 1 to 3, so far. If UAC4 is ever supported in future, it'll be highly probably the number 4. (If not and keeping the same protocol number 3, we'll need a different check in anyway.) And the other numbers are excluded already in is_supported_uac() check.
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?
The condition looks cryptic, though, yes.
Maybe the check should be factored out, e.g.
/* return true if the new config has a higher priority then the old config */ static bool check_uac_desc_priority(struct usb_host_config *old, struct usb_host_config *new) { if (!is_supported_uac(new)) return false;
if (!is_supported_uac(old)) return true;
/* * Assume that bInterfaceProtocol value is always growing; * so far, it's true from UAC1 to UAC3 (1..3) */ if (new->bInterfaceProtocol > old->bInterfaceProtocol) return true;
return false; }
thanks,
Takashi
В Вт, 13/02/2024 в 13:02 +0100, Takashi Iwai пишет:
On Tue, 13 Feb 2024 12:05:47 +0100, Greg Kroah-Hartman wrote:
On Mon, Feb 12, 2024 at 06:28:48PM +0300, Alexander Tsoy wrote:
/* If there is no UAC3 config, preferthe first config */
else if (i == 0)
if (i == 0)best = c;
/* Assume that bInterfaceProtocol valueis always
* growing when UAC versions areincremented, so that
* the direct comparison is possible. */How do we know this assumption is always true? What happens when it is not?
I believe this assumption is acceptable. It's all about the protocol number from 1 to 3, so far. If UAC4 is ever supported in future, it'll be highly probably the number 4. (If not and keeping the same protocol number 3, we'll need a different check in anyway.) And the other numbers are excluded already in is_supported_uac() check.
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?
The condition looks cryptic, though, yes.
Maybe the check should be factored out, e.g.
/* return true if the new config has a higher priority then the old config */ static bool check_uac_desc_priority(struct usb_host_config *old, struct usb_host_config *new) { if (!is_supported_uac(new)) return false;
if (!is_supported_uac(old)) return true;
/* * Assume that bInterfaceProtocol value is always growing; * so far, it's true from UAC1 to UAC3 (1..3) */ if (new->bInterfaceProtocol > old->bInterfaceProtocol) return true;
return false; }
Thank you both for response! I'll try to simplify the logic.
В Вт, 13/02/2024 в 12:05 +0100, Greg Kroah-Hartman пишет:
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@tsoy.me
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 audioso that
* devices that don't comply with UAC3 protocolare supported.
* But, still iterate through other configurationsand
* select UAC3 compliant config if present.
* Iterate through audio configurations and selectthe first of
* the highest supported UAC versions. Select thefirst config
* if no supported UAC configs are found.*/ if (desc && is_audio(desc)) {
/* Always prefer the first found UAC3config */
if (is_uac3_config(desc)) {best = c;break;}
structusb_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.
Well, similar code exists at the beginning of the loop. But I just discovered that according to the Backwards Compatibility sections of UAC 3.0 and UAC 4.0 specs, we need to look at the bFunctionProtocol field of the Interface Association descriptor. So the current code that selects UAC3 configuration is also not entirely correct AFAIS.
participants (3)
-
Alexander Tsoy -
Greg Kroah-Hartman -
Takashi Iwai