linux-kernel - Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170309002129.GA99773@google.com>
Date:   Wed, 8 Mar 2017 16:21:30 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org
Subject: Re: [PATCH for-4.11] ASoC: don't dereference NULL pcm_{new,free}

Hi Kuninori,

On Thu, Mar 09, 2017 at 12:17:41AM +0000, Kuninori Morimoto wrote:
> > Not all platform drivers have pcm_{new,free} callbacks. Seen with a
> > "snd-soc-dummy" codec from sound/soc/rockchip/rk3399_gru_sound.c.
> (snip)
> > Fixes: 99b04f4c4051 ("ASoC: add Component level pcm_new/pcm_free")
> > Signed-off-by: Brian Norris <briannorris@...omium.org>
> > ---
> > I'm really not that familiar with this subsystem... but this does fix the
> > crash seen here.
> > 
> >  sound/soc/soc-core.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index 6dca408faae3..2722bb0c5573 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -3326,7 +3326,10 @@ static int snd_soc_platform_drv_pcm_new(struct snd_soc_pcm_runtime *rtd)
> >  {
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  
> > -	return platform->driver->pcm_new(rtd);
> > +	if (platform->driver->pcm_new)
> > +		return platform->driver->pcm_new(rtd);
> > +	else
> > +		return 0;
> >  }
> >  
> >  static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> > @@ -3334,7 +3337,8 @@ static void snd_soc_platform_drv_pcm_free(struct snd_pcm *pcm)
> >  	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  
> > -	platform->driver->pcm_free(pcm);
> > +	if (platform->driver->pcm_free)
> > +		platform->driver->pcm_free(pcm);
> >  }
> 
> It is a littlle bit strange for me.

Yes, and honestly I'm a little confused by the inheritance in this
framework.

> commit 99b04f4c4051 has below code. This means, if platform doesn't have pcm_new/free callback,
> component doesn't have snd_soc_platform_drv_pcm_new/free.
> But your case, platform doesn't have pcm_new/free, but component had it ?

I have a feeling you're checking the wrong thing below for this case.
All I know is that I'm definitely hitting a NULL
platform->driver->pcm_new callback, and that either reverting your patch
or applying the patch I just sent fixes it.

Brian

> ...
> @@ -3181,6 +3198,10 @@ int snd_soc_add_platform(struct device *dev, struct snd_soc_platform *platform,
>  		platform->component.probe = snd_soc_platform_drv_probe;
>  	if (platform_drv->remove)
>  		platform->component.remove = snd_soc_platform_drv_remove;
> +	if (platform_drv->pcm_new)
> +		platform->component.pcm_new = snd_soc_platform_drv_pcm_new;
> +	if (platform_drv->pcm_free)
> +		platform->component.pcm_free = snd_soc_platform_drv_pcm_free;
>  
>  #ifdef CONFIG_DEBUG_FS
>  	platform->component.debugfs_prefix = "platform";