| Message ID | 20241121140613.3651-1-bsevens@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: Fix out of bounds reads when finding clock sources | expand |
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; }
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? On Thu, 21 Nov 2024 at 15:53, Takashi Iwai <tiwai@suse.de> wrote: > > 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; Why can't we just do this here? return cs->v3.bLength >= sizeof(cs->v3) + cs->v3.bNrInPins; > + 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; > } > > 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; > } > Thanks, Benoit
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@suse.de> 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@suse.de> 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@google.com> Link: https://lore.kernel.org/20241121140613.3651-1-bsevens@google.com Signed-off-by: Takashi Iwai <tiwai@suse.de> --- 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; }
On Fri, 22 Nov 2024 at 13:09, Takashi Iwai <tiwai@suse.de> wrote: > > 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@suse.de> 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. > Looks perfect to me. Thank you for the patch and explanations! Can we cc stable to get it backported? > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > 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@google.com> > Link: https://lore.kernel.org/20241121140613.3651-1-bsevens@google.com > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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 >
On Fri, 22 Nov 2024 14:01:53 +0100, Benoît Sevens wrote: > > On Fri, 22 Nov 2024 at 13:09, Takashi Iwai <tiwai@suse.de> wrote: > > > > 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@suse.de> 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. > > > > Looks perfect to me. Thank you for the patch and explanations! > > Can we cc stable to get it backported? Sure, I'll add it when submitting properly. thanks, Takashi
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) { + usb_audio_err(chip, + "%s(): invalid number of input pins for descriptor size, id %d\n", + __func__, clock_id); + return -EINVAL; + } + if (proto == UAC_VERSION_3) bmControls = le32_to_cpu(*(__le32 *)(&selector->v3.baCSourceID[0] + pins)); else
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(-)