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=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 9096AC32753 for ; Wed, 14 Aug 2019 15:14:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6C5D0208C2 for ; Wed, 14 Aug 2019 15:14:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728157AbfHNPO3 (ORCPT ); Wed, 14 Aug 2019 11:14:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:52262 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725955AbfHNPO2 (ORCPT ); Wed, 14 Aug 2019 11:14:28 -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 7F9E0AB92; Wed, 14 Aug 2019 15:14:26 +0000 (UTC) Date: Wed, 14 Aug 2019 17:14:25 +0200 Message-ID: From: Takashi Iwai To: Dan Carpenter Cc: Hui Peng , security@kernel.org, alsa-devel@alsa-project.org, YueHaibing , Thomas Gleixner , Allison Randal , Mathias Payer , Jaroslav Kysela , Takashi Iwai , Wenwen Wang , linux-kernel@vger.kernel.org Subject: Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit In-Reply-To: <20190814090921.GO1935@kadam> References: <20190814023625.21683-1-benquike@gmail.com> <20190814090921.GO1935@kadam> 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 Wed, 14 Aug 2019 11:09:21 +0200, Dan Carpenter wrote: > > On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote: > > On Wed, 14 Aug 2019 04:36:24 +0200, > > Hui Peng wrote: > > > > > > The `uac_mixer_unit_descriptor` shown as below is read from the > > > device side. In `parse_audio_mixer_unit`, `baSourceID` field is > > > accessed from index 0 to `bNrInPins` - 1, the current implementation > > > assumes that descriptor is always valid (the length of descriptor > > > is no shorter than 5 + `bNrInPins`). If a descriptor read from > > > the device side is invalid, it may trigger out-of-bound memory > > > access. > > > > > > ``` > > > struct uac_mixer_unit_descriptor { > > > __u8 bLength; > > > __u8 bDescriptorType; > > > __u8 bDescriptorSubtype; > > > __u8 bUnitID; > > > __u8 bNrInPins; > > > __u8 baSourceID[]; > > > } > > > ``` > > > > > > This patch fixes the bug by add a sanity check on the length of > > > the descriptor. > > > > > > Signed-off-by: Hui Peng > > > Reported-by: Hui Peng > > > Reported-by: Mathias Payer > > > --- > > > sound/usb/mixer.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c > > > index 7498b5191b68..38202ce67237 100644 > > > --- a/sound/usb/mixer.c > > > +++ b/sound/usb/mixer.c > > > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid, > > > struct usb_audio_term iterm; > > > int input_pins, num_ins, num_outs; > > > int pin, ich, err; > > > + int desc_len = (int) ((unsigned long) state->buffer + > > > + state->buflen - (unsigned long) raw_desc); > > > + > > > + if (desc_len < sizeof(*desc) + desc->bNrInPins) { > > > + usb_audio_err(state->chip, > > > + "descriptor %d too short\n", > > > + unitid); > > > + return -EINVAL; > > > + } > > > > > > err = uac_mixer_unit_get_channels(state, desc); > > > if (err < 0) { > > > > Hm, what is the desc->bLength value in the error case? > > > > Basically the buffer boundary is already checked against bLength in > > snd_usb_find_desc() which is called from obtaining the raw_desc in the > > caller of this function (parse_audio_unit()). > > > > So, if any, we need to check bLength for the possible overflow like > > below. > > > > > > thanks, > > > > Takashi > > > > --- a/sound/usb/mixer.c > > +++ b/sound/usb/mixer.c > > @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, > > return -EINVAL; > > if (!desc->bNrInPins) > > return -EINVAL; > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins) > > + return -EINVAL; > > VERSION 1 and 2 already have a different check: > > if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1) > return 0; /* no bmControls -> skip */ > > So something is possibly off by one. It's just version 3 which doesn't > have a check. > No, both are sensible checks. The first check is about the minimal size that doesn't contain bmControls bitmap which is optional on some devices, while the latter checks about the presence of bmControls field. Note that the latter returns zero, which means no error, while the former returns an error. thanks, Takashi