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

[Takashi Iwai <tiwai@xxxxxxx>]

On Fri, 22 Nov 2024 11:06:57 +0100,
Benoît Sevens wrote:
> 
> Hi Takashi,
> 
> Thank you for the review. Except for one question below, this patch
> looks good to me. Would you like me to send your patch as a revised
> patch to this mailing list?

I'm going to submit the proper patch if the fix works for you.

> On Thu, 21 Nov 2024 at 15:53, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > @@ -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;
> 
> Why can't we just do this here?
> 
>  return cs->v3.bLength >= sizeof(cs->v3) + cs->v3.bNrInPins;

Argh, it was just a wrong check.

But the clock selector descriptor must have two more fields in
addition to baCSourceID array.  Those two are 6 bytes (= 4 bytes for
bmControls + 2 bytes for wCSelectorDescrStr) for UAC3, while...

> > +       else
> > +               return cs->v2.bLength >= sizeof(cs->v2.bLength) + cs->v2.bNrInPins + 2;
> 
> And same question here, why not:
> 
>  return cs->v2.bLength >= sizeof(cs->v2) + cs->v2.bNrInPins;

... for UAC2, they are 2 bytes (= 1 byte for bmControls + 1 byte for
iClockSelector).  So those numbers appeared in the patch.

Below is the revised patch.  Let me know if this works for you.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] ALSA: usb-audio: Fix out of bounds reads when finding clock sources

The current USB-audio driver code doesn't check bLength of each
descriptor at traversing for clock descriptors.  That is, when a
device provides a bogus descriptor with a shorter bLength, the driver
might hit out-of-bounds reads.

For addressing it, this patch adds sanity checks to the validator
functions for the clock descriptor traversal.  When the descriptor
length is shorter than expected, it's skipped in the loop.

For the clock source and clock multiplier descriptors, we can just
check bLength against the sizeof() of each descriptor type.
OTOH, the clock selector descriptor of UAC2 and UAC3 has an array
of bNrInPins elements and two more fields at its tail, hence those
have to be checked in addition to the sizeof() check.

Reported-by: Benoît Sevens <bsevens@xxxxxxxxxx>
Link: https://lore.kernel.org/20241121140613.3651-1-bsevens@xxxxxxxxxx
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 sound/usb/clock.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 8f85200292f3..842ba5b801ea 100644
--- 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,27 @@ 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 array (in bNrInPins size)
+	 * and two more fields (which sizes depend on the protocol)
+	 */
+	if (proto == UAC_VERSION_3)
+		return cs->v3.bLength >= sizeof(cs->v3) + cs->v3.bNrInPins +
+			4 /* bmControls */ + 2 /* wCSelectorDescrStr */;
+	else
+		return cs->v2.bLength >= sizeof(cs->v2) + cs->v2.bNrInPins +
+			1 /* bmControls */ + 1 /* iClockSelector */;
 }
 
 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;
 }
 
-- 
2.43.0