linux-kernel - Re: [PATCH] ALSA: control: Fix memory corruption risk in snd_ctl_elem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <s5ho9kanz2t.wl-tiwai@suse.de>
Date:   Tue, 27 Feb 2018 18:04:42 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     "Richard Fitzgerald" <rf@...nsource.cirrus.com>
Cc:     <perex@...ex.cz>, <o-takashi@...amocchi.jp>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

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 <rf@...nsource.cirrus.com>
> ---
>  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