Re: [PATCH] ALSA: usb-audio: Fix out of bounds reads when finding clock sources

[Takashi Iwai <tiwai@suse.de>]

On Thu, 21 Nov 2024 15:06:13 +0100,
Benoît Sevens wrote:
> 
> A bogus device can provide a clock selector descriptor that contains a
> bNrInPins that is larger than the actual size of baCSourceID. This can
> lead to out-of-bound reads in __uac_clock_find_source. These out-of-bound
> values can be leaked back to the device via the uac_clock_selector_get_val
> calls.
> 
> Fixes: 79f920fbff56 ("ALSA: usb-audio: parse clock topology of UAC2 devices")
> CC: stable@kernel.org
> Signed-off-by: Benoît Sevens <bsevens@google.com>
> ---
>  sound/usb/clock.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 8f85200292f3..94fb628f116e 100644
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -270,7 +270,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  	union uac23_clock_source_desc *source;
>  	union uac23_clock_selector_desc *selector;
>  	union uac23_clock_multiplier_desc *multiplier;
> -	int ret, i, cur, err, pins, clock_id;
> +	int ret, i, cur, err, length, pins, clock_id;
>  	const u8 *sources;
>  	int proto = fmt->protocol;
>  	bool readable, writeable;
> @@ -301,11 +301,19 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
>  
>  	selector = snd_usb_find_clock_selector(chip, entity_id, fmt);
>  	if (selector) {
> +		length = GET_VAL(selector, proto, bLength);
>  		pins = GET_VAL(selector, proto, bNrInPins);
>  		clock_id = GET_VAL(selector, proto, bClockID);
>  		sources = GET_VAL(selector, proto, baCSourceID);
>  		cur = 0;
>  
> +		if (length < sizeof(selector) + pins) {

This happens to be correct because the size of both
uac_clock_selector_descriptor and uac3_clock_selector_descriptor are
same.  Otherwise we'd have to pass sizeof() of the corresponding
struct depending on the proto value.  So it's worth to comment here.

BTW, now I noticed that we didn't check the size at traversing the
descriptors.  This means that the driver might read at a wrong
position even before the point this patch addresses.  For covering it,
we need additional checks in the validator code.  And this size check
of clock selectors can be put there, too.

That is, something like below (totally untested).

It'll lead to a different behavior from your patch, though; currently,
when no suitable clock selector is found, it check the multiplier and
this might match.  So I'm not entirely sure, but just wanted to tell
you that it's another option.


thanks,

Takashi

--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -36,6 +36,12 @@ union uac23_clock_multiplier_desc {
 	struct uac_clock_multiplier_descriptor v3;
 };
 
+/* check whether the descriptor bLength has the minimal length */
+#define DESC_LENGTH_CHECK(p, proto) \
+	((proto) == UAC_VERSION_3 ? \
+	 ((p)->v3.bLength >= sizeof((p)->v3)) :	\
+	 ((p)->v2.bLength >= sizeof((p)->v2)))
+
 #define GET_VAL(p, proto, field) \
 	((proto) == UAC_VERSION_3 ? (p)->v3.field : (p)->v2.field)
 
@@ -58,6 +64,8 @@ static bool validate_clock_source(void *p, int id, int proto)
 {
 	union uac23_clock_source_desc *cs = p;
 
+	if (!DESC_LENGTH_CHECK(cs, proto))
+		return false;
 	return GET_VAL(cs, proto, bClockID) == id;
 }
 
@@ -65,13 +73,23 @@ static bool validate_clock_selector(void *p, int id, int proto)
 {
 	union uac23_clock_selector_desc *cs = p;
 
-	return GET_VAL(cs, proto, bClockID) == id;
+	if (!DESC_LENGTH_CHECK(cs, proto))
+		return false;
+	if (GET_VAL(cs, proto, bClockID) != id)
+		return false;
+	/* additional length check for baCSourceID, bmControls and iClockSelector */
+	if (proto == UAC_VERSION_3)
+		return cs->v3.bLength >= sizeof(cs->v3.bLength) + cs->v3.bNrInPins + 6;
+	else
+		return cs->v2.bLength >= sizeof(cs->v2.bLength) + cs->v2.bNrInPins + 2;
 }
 
 static bool validate_clock_multiplier(void *p, int id, int proto)
 {
 	union uac23_clock_multiplier_desc *cs = p;
 
+	if (!DESC_LENGTH_CHECK(cs, proto))
+		return false;
 	return GET_VAL(cs, proto, bClockID) == id;
 }