From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752207AbeB0REq (ORCPT ); Tue, 27 Feb 2018 12:04:46 -0500 Received: from mx2.suse.de ([195.135.220.15]:47600 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751919AbeB0REo (ORCPT ); Tue, 27 Feb 2018 12:04:44 -0500 Date: Tue, 27 Feb 2018 18:04:42 +0100 Message-ID: From: Takashi Iwai To: "Richard Fitzgerald" Cc: , , , , Subject: Re: [PATCH] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read In-Reply-To: <20180227170118.23197-1-rf@opensource.cirrus.com> References: <20180227170118.23197-1-rf@opensource.cirrus.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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 27 Feb 2018 18:01:18 +0100, Richard Fitzgerald wrote: > > The patch "ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE > operations" introduced a potential for kernel memory corruption due > to an incorrect if statement allowing non-readable controls to fall > through and call the get function. For TLV controls a driver can omit > SNDRV_CTL_ELEM_ACCESS_READ to ensure that only the TLV get function > can be called. Instead the normal get() can be invoked unexpectedly > and as the driver expects that this will only be called for controls > <= 512 bytes, potentially try to copy >512 bytes into the 512 byte > return array, so corrupting kernel memory. > > The problem is an attempt to refactor the snd_ctl_elem_read function > to invert the logic so that it conditionally aborted if the control > is unreadable instead of conditionally executing. But the if statement > wasn't inverted correctly. > > The correct inversion of > > if (a && !b) > > is > if (!a || b) > > Fixes: becf9e5d553c2 ("ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations") > Signed-off-by: Richard Fitzgerald > --- > sound/core/control.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/core/control.c b/sound/core/control.c > index 0b3026d937b1..8a77620a3854 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -889,7 +889,7 @@ static int snd_ctl_elem_read(struct snd_card *card, > > index_offset = snd_ctl_get_ioff(kctl, &control->id); > vd = &kctl->vd[index_offset]; > - if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get == NULL) > + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) || kctl->get == NULL) Doh, it's a common mistake. Thanks for catching this. It deserves for stable kernel. Takashi