From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 10ABAC31E40 for ; Thu, 15 Aug 2019 06:14:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D919C2133F for ; Thu, 15 Aug 2019 06:14:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730308AbfHOGOA (ORCPT ); Thu, 15 Aug 2019 02:14:00 -0400 Received: from mx2.suse.de ([195.135.220.15]:38584 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725977AbfHOGOA (ORCPT ); Thu, 15 Aug 2019 02:14:00 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 01415AC68; Thu, 15 Aug 2019 06:13:57 +0000 (UTC) Date: Thu, 15 Aug 2019 08:13:57 +0200 Message-ID: From: Takashi Iwai To: "Hui Peng" Cc: , , "YueHaibing" , "Thomas Gleixner" , "Allison Randal" , "Mathias Payer" , "Jaroslav Kysela" , "Takashi Iwai" , "Wenwen Wang" , Subject: Re: [PATCH] Fix a stack buffer overflow bug check_input_term In-Reply-To: <20190815043554.16623-1-benquike@gmail.com> References: <20190815043554.16623-1-benquike@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? 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 */