Re: [PATCH] ALSA: control: Fix memory corruption risk in snd_ctl_elem_read

[Takashi Iwai]

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@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  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