[alsa-devel] PATCH: ALSA: hda: fix possible null dereference

[Takashi Iwai]

On Mon, 24 Aug 2015 12:17:40 +0200,
Markus Osterhoff wrote:
> 
> Dear Takashi,
> 
> maybe I am mistaken or have not made my patch clear, please let me
> apologise.
> 
> * Takashi Iwai <tiwai at suse.de> [150824 08:30]:
> > On Sun, 23 Aug 2015 20:37:05 +0200,
> > Markus Osterhoff wrote:
> > > With commits bbbc7e85 and 751e2216, a for-loop was restated using
> > > list_for_each_entry(...); originally, a local pcm* was extracted from an array
> > > and checked against NULL, if not-NULL then passed along to
> > > snd_pcm_add_chmap_ctls(...).
> > > 
> > > For a history of the function in question, have a look at lines 2430ff for
> > > 	git diff v4.0 v4.1 sound/pci/hda/hda_codec.c
> > > 
> > > The two commits that touched this function are
> > > Commit bbbc7e8502c9 ("ALSA: hda - Allocate hda_pcm objects dynamically")
> > > Commit 751e2216899c ("ALSA: hda: fix possible null dereference")
> > > 
> > > where the latter only fixes the order of two fixes, but not the pcm / pcm->pcm confusion.
> 
> > Thanks for the patch.  But this was already fixed by commit
> > 751e2216899c in 4.1-rc1.
> So I re-checked and diff'ed against the current v4.2-rc8 and found that
> the if-statement reads, /sound/pci/hda/hda_codec.c, line 3172:
> 	if (!pcm || pcm->own_chmap ||
> while I propose
> 	if (!pcm->pcm || pcm->own_chmap ||

Then your previous patch did exactly opposite :)

> With the mentioned 751e2216899c I do get a NULL dereference (for
> whatever reason), because pcm->pcm can indeed by NULL inside the loop. Please
> reconsider the patch:

Actually you're trying to shoot two things in a shot, and it's
confusing.  What we need is to *add* the NULL check of pcm->pcm.  It's
basically different from replacing pcm NULL check itself.
The pcm NULL check is superfluous, indeed, and can be removed.  But
it can be done in a different patch, or mention properly in the
changelog.

Could you revisit your patch with the proper description?


thanks,

Takashi

> 
> > Takashi
> Thanks for your time, Markus
> 
> 
> Signed-off-by: Markus Osterhoff <linux-kernel at k-raum.org>
> ---
>  sound/pci/hda/hda_codec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 5de3c5d..47206b9 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -3172,7 +3172,7 @@ static int add_std_chmaps(struct hda_codec *codec)
>                         struct snd_pcm_chmap *chmap;
>                         const struct snd_pcm_chmap_elem *elem;
>  
> -                       if (!pcm || pcm->own_chmap ||
> +                       if (!pcm->pcm || pcm->own_chmap ||
>                             !hinfo->substreams)
>                                 continue;
>                         elem = hinfo->chmap ? hinfo->chmap : snd_pcm_std_chmaps;
>