[alsa-devel] [PATCH] Fix a stack buffer overflow bug check_input_term
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com --- sound/usb/mixer.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..1f6c8213df82 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS); + DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map; @@ -782,6 +783,8 @@ static int check_input_term(struct mixer_build *state, int id, int err; void *p1;
+ if (test_and_set_bit(id, state->termbitmap)) + return 0; memset(term, 0, sizeof(*term)); while ((p1 = find_audio_control_unit(state, id)) != NULL) { unsigned char *hdr = p1;
The stack trace differs from test to test, the attached trace1 file is taken from one of the tests.
The bug is confirmed by adding some printk statement in `check_input_term`, the trace with output of printk is attached in trace2 file.
This patch is a tentative fix to the bug, please give me feedback.
On 8/15/19 12:35 AM, Hui Peng wrote:
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com
sound/usb/mixer.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..1f6c8213df82 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS);
- DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map;
@@ -782,6 +783,8 @@ static int check_input_term(struct mixer_build *state, int id, int err; void *p1;
- if (test_and_set_bit(id, state->termbitmap))
return 0;
On Thu, 15 Aug 2019 06:35:49 +0200, Hui Peng wrote:
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com
The fix looks almost good, but we need to be careful about the bitmap check. In theory, it's possible that multiple nodes point to the same input terminal, and your patch would break that scenario. For fixing that, we need to zero-clear the termbitmap at each first invocation of check_input_term(), something like below.
Could you check whether this works?
thanks,
Takashi
--- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS); + DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map; @@ -773,14 +774,15 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, * parse the source unit recursively until it reaches to a terminal * or a branched unit. */ -static int check_input_term(struct mixer_build *state, int id, - struct usb_audio_term *term) +static int __check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) { int protocol = state->mixer->protocol; int err; void *p1;
- memset(term, 0, sizeof(*term)); + if (test_and_set_bit(id, state->termbitmap)) + return 0; while ((p1 = find_audio_control_unit(state, id)) != NULL) { unsigned char *hdr = p1; term->id = id; @@ -800,7 +802,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d->bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err;
@@ -834,7 +836,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC2_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -897,7 +899,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d->bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err;
@@ -948,7 +950,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC3_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -964,7 +966,7 @@ static int check_input_term(struct mixer_build *state, int id, return -EINVAL;
/* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err;
@@ -982,6 +984,14 @@ static int check_input_term(struct mixer_build *state, int id, return -ENODEV; }
+static int check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) +{ + memset(term, 0, sizeof(*term)); + memset(state->termbitmap, 0, sizeof(state->termbitmap)); + return __check_input_term(state, id, term); +} + /* * Feature Unit */
On Thu, 15 Aug 2019 08:13:57 +0200, Takashi Iwai wrote:
On Thu, 15 Aug 2019 06:35:49 +0200, Hui Peng wrote:
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com
The fix looks almost good, but we need to be careful about the bitmap check. In theory, it's possible that multiple nodes point to the same input terminal, and your patch would break that scenario. For fixing that, we need to zero-clear the termbitmap at each first invocation of check_input_term(), something like below.
Could you check whether this works?
Thinking of this further, there is another possible infinite loop. Namely, when the feature unit in the input terminal chain points to itself as the source, it'll loop endlessly without the stack overflow.
So the check of the termbitmap should be inside the loop. The revised patch is below.
thanks,
Takashi
-- 8< -- From: Hui Peng benquike@gmail.com Subject: [PATCH] ALSA: usb-audio: Fix a stack buffer overflow bug check_input_term
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
[ The termbitmap needs to be cleared at each first check of the input terminal, so the function got split now. Also, for catching another endless loop in the input terminal chain -- where the feature unit points to itself as its source -- the termbitmap check is moved inside the parser loop. -- tiwai ]
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/mixer.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..aa8b046aa91f 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS); + DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map; @@ -775,16 +776,23 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, * parse the source unit recursively until it reaches to a terminal * or a branched unit. */ -static int check_input_term(struct mixer_build *state, int id, - struct usb_audio_term *term) +static int __check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) { int protocol = state->mixer->protocol; int err; void *p1; + unsigned char *hdr;
- memset(term, 0, sizeof(*term)); - while ((p1 = find_audio_control_unit(state, id)) != NULL) { - unsigned char *hdr = p1; + for (;;) { + /* a loop in the terminal chain? */ + if (test_and_set_bit(id, state->termbitmap)) + break; + + p1 = find_audio_control_unit(state, id); + if (!p1) + break; + hdr = p1; term->id = id;
if (protocol == UAC_VERSION_1 || protocol == UAC_VERSION_2) { @@ -802,7 +810,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d->bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err;
@@ -836,7 +844,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC2_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -899,7 +907,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d->bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err;
@@ -950,7 +958,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC3_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -966,7 +974,7 @@ static int check_input_term(struct mixer_build *state, int id, return -EINVAL;
/* call recursively to retrieve the channel info */ - err = check_input_term(state, d->baSourceID[0], term); + err = __check_input_term(state, d->baSourceID[0], term); if (err < 0) return err;
@@ -984,6 +992,14 @@ static int check_input_term(struct mixer_build *state, int id, return -ENODEV; }
+static int check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) +{ + memset(term, 0, sizeof(*term)); + memset(state->termbitmap, 0, sizeof(state->termbitmap)); + return __check_input_term(state, id, term); +} + /* * Feature Unit */
Hi, Takashi:
One point I want to be clear: if an endless recursive loop is detected, should we return 0, or a negative error code?
On Thu, Aug 15, 2019 at 2:58 AM Takashi Iwai tiwai@suse.de wrote:
On Thu, 15 Aug 2019 08:13:57 +0200, Takashi Iwai wrote:
On Thu, 15 Aug 2019 06:35:49 +0200, Hui Peng wrote:
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com
The fix looks almost good, but we need to be careful about the bitmap check. In theory, it's possible that multiple nodes point to the same input terminal, and your patch would break that scenario. For fixing that, we need to zero-clear the termbitmap at each first invocation of check_input_term(), something like below.
Could you check whether this works?
Thinking of this further, there is another possible infinite loop. Namely, when the feature unit in the input terminal chain points to itself as the source, it'll loop endlessly without the stack overflow.
So the check of the termbitmap should be inside the loop. The revised patch is below.
thanks,
Takashi
-- 8< -- From: Hui Peng benquike@gmail.com Subject: [PATCH] ALSA: usb-audio: Fix a stack buffer overflow bug check_input_term
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
[ The termbitmap needs to be cleared at each first check of the input terminal, so the function got split now. Also, for catching another endless loop in the input terminal chain -- where the feature unit points to itself as its source -- the termbitmap check is moved inside the parser loop. -- tiwai ]
Reported-by: Hui Peng benquike@gmail.com Reported-by: Mathias Payer mathias.payer@nebelwelt.net Signed-off-by: Hui Peng benquike@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
sound/usb/mixer.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..aa8b046aa91f 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS);
DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map;@@ -775,16 +776,23 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
- parse the source unit recursively until it reaches to a terminal
- or a branched unit.
*/ -static int check_input_term(struct mixer_build *state, int id,
struct usb_audio_term *term)+static int __check_input_term(struct mixer_build *state, int id,
struct usb_audio_term *term){ int protocol = state->mixer->protocol; int err; void *p1;
unsigned char *hdr;
memset(term, 0, sizeof(*term));while ((p1 = find_audio_control_unit(state, id)) != NULL) {unsigned char *hdr = p1;
for (;;) {/* a loop in the terminal chain? */if (test_and_set_bit(id, state->termbitmap))break;p1 = find_audio_control_unit(state, id);if (!p1)break;hdr = p1; term->id = id; if (protocol == UAC_VERSION_1 || protocol ==UAC_VERSION_2) { @@ -802,7 +810,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify thatthe * referenced clock entity is valid */
err = check_input_term(state,d->bCSourceID, term);
err = __check_input_term(state,d->bCSourceID, term); if (err < 0) return err;
@@ -836,7 +844,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC2_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */
err = check_input_term(state,d->baSourceID[0], term);
err = __check_input_term(state,d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -899,7 +907,7 @@ static int check_input_term(struct mixer_build *state, int id,
/* call recursively to verify that the * referenced clock entity is valid */
err = check_input_term(state,d->bCSourceID, term);
err = __check_input_term(state,d->bCSourceID, term); if (err < 0) return err;
@@ -950,7 +958,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC3_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */
err = check_input_term(state,d->baSourceID[0], term);
err = __check_input_term(state,d->baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -966,7 +974,7 @@ static int check_input_term(struct mixer_build *state, int id, return -EINVAL;
/* call recursively to retrieve thechannel info */
err = check_input_term(state,d->baSourceID[0], term);
err = __check_input_term(state,d->baSourceID[0], term); if (err < 0) return err;
@@ -984,6 +992,14 @@ static int check_input_term(struct mixer_build *state, int id, return -ENODEV; }
+static int check_input_term(struct mixer_build *state, int id,
struct usb_audio_term *term)+{
memset(term, 0, sizeof(*term));memset(state->termbitmap, 0, sizeof(state->termbitmap));return __check_input_term(state, id, term);+}
/*
- Feature Unit
*/
2.16.4
On Thu, 15 Aug 2019 19:19:10 +0200, Hui Peng wrote:
Hi, Takashi:
One point I want to be clear: if an endless recursive loop is detected, should we return 0, or a negative error code?
An error might be more appropriate, but it's no big deal, as you'll likely hit other errors sooner or later at parsing further :)
thanks,
Takashi
On Thu, Aug 15, 2019 at 2:58 AM Takashi Iwai tiwai@suse.de wrote:
On Thu, 15 Aug 2019 08:13:57 +0200, Takashi Iwai wrote: > > On Thu, 15 Aug 2019 06:35:49 +0200, > Hui Peng wrote: > > > > `check_input_term` recursively calls itself with input > > from device side (e.g., uac_input_terminal_descriptor.bCSourceID) > > as argument (id). In `check_input_term`, if `check_input_term` > > is called with the same `id` argument as the caller, it triggers > > endless recursive call, resulting kernel space stack overflow. > > > > This patch fixes the bug by adding a bitmap to `struct mixer_build` > > to keep track of the checked ids by `check_input_term` and stop > > the execution if some id has been checked (similar to how > > parse_audio_unit handles unitid argument). > > > > Reported-by: Hui Peng <benquike@gmail.com> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net> > > Signed-off-by: Hui Peng <benquike@gmail.com> > > The fix looks almost good, but we need to be careful about the > bitmap check. In theory, it's possible that multiple nodes point to > the same input terminal, and your patch would break that scenario. > For fixing that, we need to zero-clear the termbitmap at each first > invocation of check_input_term(), something like below. > > Could you check whether this works? Thinking of this further, there is another possible infinite loop. Namely, when the feature unit in the input terminal chain points to itself as the source, it'll loop endlessly without the stack overflow. So the check of the termbitmap should be inside the loop. The revised patch is below. thanks, Takashi -- 8< -- From: Hui Peng <benquike@gmail.com> Subject: [PATCH] ALSA: usb-audio: Fix a stack buffer overflow bug check_input_term `check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow. This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument). [ The termbitmap needs to be cleared at each first check of the input terminal, so the function got split now. Also, for catching another endless loop in the input terminal chain -- where the feature unit points to itself as its source -- the termbitmap check is moved inside the parser loop. -- tiwai ] Reported-by: Hui Peng <benquike@gmail.com> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net> Signed-off-by: Hui Peng <benquike@gmail.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/mixer.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..aa8b046aa91f 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS); + DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map; @@ -775,16 +776,23 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, * parse the source unit recursively until it reaches to a terminal * or a branched unit. */ -static int check_input_term(struct mixer_build *state, int id, - struct usb_audio_term *term) +static int __check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) { int protocol = state->mixer->protocol; int err; void *p1; + unsigned char *hdr; - memset(term, 0, sizeof(*term)); - while ((p1 = find_audio_control_unit(state, id)) != NULL) { - unsigned char *hdr = p1; + for (;;) { + /* a loop in the terminal chain? */ + if (test_and_set_bit(id, state->termbitmap)) + break; + + p1 = find_audio_control_unit(state, id); + if (!p1) + break; + hdr = p1; term->id = id; if (protocol == UAC_VERSION_1 || protocol == UAC_VERSION_2) { @@ -802,7 +810,7 @@ static int check_input_term(struct mixer_build *state, int id, /* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d-> bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err; @@ -836,7 +844,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC2_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -899,7 +907,7 @@ static int check_input_term(struct mixer_build *state, int id, /* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d-> bCSourceID, term); + err = __check_input_term(state, d-> bCSourceID, term); if (err < 0) return err; @@ -950,7 +958,7 @@ static int check_input_term(struct mixer_build *state, int id, case UAC3_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -966,7 +974,7 @@ static int check_input_term(struct mixer_build *state, int id, return -EINVAL; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err; @@ -984,6 +992,14 @@ static int check_input_term(struct mixer_build *state, int id, return -ENODEV; } +static int check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) +{ + memset(term, 0, sizeof(*term)); + memset(state->termbitmap, 0, sizeof(state->termbitmap)); + return __check_input_term(state, id, term); +} + /* * Feature Unit */ -- 2.16.4
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi, Takashi:
The new patch is confirmed (I made it to return -EINVAL if a endless recursive call is detected). Can you have a look.
Thanks.
On 8/15/19 1:38 PM, Takashi Iwai wrote:
On Thu, 15 Aug 2019 19:19:10 +0200, Hui Peng wrote:
Hi, Takashi:
One point I want to be clear: if an endless recursive loop is
detected, should
we return 0, or a negative error code?
An error might be more appropriate, but it's no big deal, as you'll likely hit other errors sooner or later at parsing further :)
thanks,
Takashi
On Thu, Aug 15, 2019 at 2:58 AM Takashi Iwai wrote:
On Thu, 15 Aug 2019 08:13:57 +0200, Takashi Iwai wrote:
On Thu, 15 Aug 2019 06:35:49 +0200, Hui Peng wrote:
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
Reported-by: Hui Peng Reported-by: Mathias Payer Signed-off-by: Hui Peng
The fix looks almost good, but we need to be careful about the bitmap check. In theory, it's possible that multiple nodes point to the same input terminal, and your patch would break that scenario. For fixing that, we need to zero-clear the termbitmap at each first invocation of check_input_term(), something like below.
Could you check whether this works?
Thinking of this further, there is another possible infinite loop. Namely, when the feature unit in the input terminal chain points to itself as the source, it'll loop endlessly without the stack overflow.
So the check of the termbitmap should be inside the loop. The revised patch is below.
thanks,
Takashi
-- 8< -- From: Hui Peng Subject: [PATCH] ALSA: usb-audio: Fix a stack buffer overflow bug check_input_term
`check_input_term` recursively calls itself with input from device side (e.g., uac_input_terminal_descriptor.bCSourceID) as argument (id). In `check_input_term`, if `check_input_term` is called with the same `id` argument as the caller, it triggers endless recursive call, resulting kernel space stack overflow.
This patch fixes the bug by adding a bitmap to `struct mixer_build` to keep track of the checked ids by `check_input_term` and stop the execution if some id has been checked (similar to how parse_audio_unit handles unitid argument).
[ The termbitmap needs to be cleared at each first check of the input terminal, so the function got split now. Also, for catching another endless loop in the input terminal chain -- where the feature unit points to itself as its source -- the termbitmap check is moved inside the parser loop. -- tiwai ]
Reported-by: Hui Peng Reported-by: Mathias Payer Signed-off-by: Hui Peng Cc: Signed-off-by: Takashi Iwai
sound/usb/mixer.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index ea487378be17..aa8b046aa91f 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -68,6 +68,7 @@ struct mixer_build { unsigned char *buffer; unsigned int buflen; DECLARE_BITMAP(unitbitmap, MAX_ID_ELEMS); + DECLARE_BITMAP(termbitmap, MAX_ID_ELEMS); struct usb_audio_term oterm; const struct usbmix_name_map *map; const struct usbmix_selector_map *selector_map; @@ -775,16 +776,23 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, * parse the source unit recursively until it reaches to a terminal * or a branched unit. */ -static int check_input_term(struct mixer_build *state, int id, - struct usb_audio_term *term) +static int __check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) { int protocol = state->mixer->protocol; int err; void *p1; + unsigned char *hdr;
- memset(term, 0, sizeof(*term)); - while ((p1 = find_audio_control_unit(state, id)) != NULL) { - unsigned char *hdr = p1; + for (;;) { + /* a loop in the terminal chain? */ + if (test_and_set_bit(id, state->termbitmap)) + break;
+ p1 = find_audio_control_unit(state, id); + if (!p1) + break; + hdr = p1; term->id = id;
if (protocol == UAC_VERSION_1 || protocol == UAC_VERSION_2) { @@ -802,7 +810,7 @@ static int check_input_term(struct mixer_build
*state,
int id,
/* call recursively to verify
that
the * referenced clock entity is valid */ - err = check_input_term(state, d-> bCSourceID, term); + err = __check_input_term(state, d->bCSourceID, term); if (err < 0) return err;
@@ -836,7 +844,7 @@ static int check_input_term(struct mixer_build
*state,
int id, case UAC2_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -899,7 +907,7 @@ static int check_input_term(struct mixer_build
*state,
int id,
/* call recursively to verify that the * referenced clock entity is valid */ - err = check_input_term(state, d-> bCSourceID, term); + err = __check_input_term(state, d-> bCSourceID, term); if (err < 0) return err;
@@ -950,7 +958,7 @@ static int check_input_term(struct mixer_build
*state,
int id, case UAC3_CLOCK_SELECTOR: { struct uac_selector_unit_descriptor *d = p1; /* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err; term->type = UAC3_SELECTOR_UNIT << 16; /* virtual type */ @@ -966,7 +974,7 @@ static int check_input_term(struct mixer_build
*state,
int id, return -EINVAL;
/* call recursively to retrieve the channel info */ - err = check_input_term(state, d-> baSourceID[0], term); + err = __check_input_term(state, d-> baSourceID[0], term); if (err < 0) return err;
@@ -984,6 +992,14 @@ static int check_input_term(struct mixer_build *state, int id, return -ENODEV; }
+static int check_input_term(struct mixer_build *state, int id, + struct usb_audio_term *term) +{ + memset(term, 0, sizeof(*term)); + memset(state->termbitmap, 0, sizeof(state->termbitmap)); + return __check_input_term(state, id, term); +}
/* * Feature Unit */ -- 2.16.4
participants (2)
-
Hui Peng -
Takashi Iwai