[alsa-devel] PATCH: ALSA: hda: fix possible null dereference
Upgrading on Gentoo hardened from 4.0.8 to 4.1.6 (with intermediate 4.1.4), I noticed a NULL pointer derefence during booting, and tackled it down to static int add_std_chmaps(struct hda_codec *codec) in file sound/pci/hda/hda_codec.c
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(...).
Now, pcm->pcm is passed to snd_pcm_add_chmap_ctls, but the NULL pointer check still uses the “upper level” pcm, not the “in-loop” pcm->pcm.
Please have a look at the attached (rather small) suggestion to fix this problem.
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.
Using v4.1-descendant sources, boot got OOPS like this (two bug-hunting printk's added up front):
pcm: ffff88040cf83400 pcm->pcm: (nil) BUG: unable to handle kernel NULL pointer dereference at 0000000000000018 IP: [<ffffffffa07feab9>] snd_pcm_add_chmap_ctls+0x119/0x1f0 [snd_pcm] PGD 0 Oops: 0000 [#1] PREEMPT SMP
NULL checking pcm->pcm instead of pcm fixes this problem.
Greetings and thanks for your time, Markus
Signed-off-by: Markus Osterhoff linux-kernel@k-raum.org --- --- sound/pci/hda/hda_codec.c 2015-08-23 20:02:57.281782648 +0200 +++ sound/pci/hda/hda_codec.c.orig 2015-08-23 20:02:49.723783063 +0200 @@ -3172,7 +3172,7 @@ static int add_std_chmaps(struct hda_cod struct snd_pcm_chmap *chmap; const struct snd_pcm_chmap_elem *elem;
- if (!pcm->pcm || pcm->own_chmap || + if (!pcm || pcm->own_chmap || !hinfo->substreams) continue; elem = hinfo->chmap ? hinfo->chmap : snd_pcm_std_chmaps;
On Sun, 23 Aug 2015 20:37:05 +0200, Markus Osterhoff wrote:
Upgrading on Gentoo hardened from 4.0.8 to 4.1.6 (with intermediate 4.1.4), I noticed a NULL pointer derefence during booting, and tackled it down to static int add_std_chmaps(struct hda_codec *codec) in file sound/pci/hda/hda_codec.c
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(...).
Now, pcm->pcm is passed to snd_pcm_add_chmap_ctls, but the NULL pointer check still uses the “upper level” pcm, not the “in-loop” pcm->pcm.
Please have a look at the attached (rather small) suggestion to fix this problem.
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.
Using v4.1-descendant sources, boot got OOPS like this (two bug-hunting printk's added up front):
pcm: ffff88040cf83400 pcm->pcm: (nil) BUG: unable to handle kernel NULL pointer dereference at 0000000000000018 IP: [<ffffffffa07feab9>] snd_pcm_add_chmap_ctls+0x119/0x1f0 [snd_pcm] PGD 0 Oops: 0000 [#1] PREEMPT SMP
NULL checking pcm->pcm instead of pcm fixes this problem.
Greetings and thanks for your time, Markus
Signed-off-by: Markus Osterhoff linux-kernel@k-raum.org
Thanks for the patch. But this was already fixed by commit 751e2216899c in 4.1-rc1.
Takashi
--- sound/pci/hda/hda_codec.c 2015-08-23 20:02:57.281782648 +0200 +++ sound/pci/hda/hda_codec.c.orig 2015-08-23 20:02:49.723783063 +0200 @@ -3172,7 +3172,7 @@ static int add_std_chmaps(struct hda_cod struct snd_pcm_chmap *chmap; const struct snd_pcm_chmap_elem *elem;
if (!pcm->pcm || pcm->own_chmap ||
if (!pcm || pcm->own_chmap || !hinfo->substreams) continue; elem = hinfo->chmap ? hinfo->chmap : snd_pcm_std_chmaps;
Dear Takashi,
maybe I am mistaken or have not made my patch clear, please let me apologise.
* Takashi Iwai tiwai@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 ||
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:
Takashi
Thanks for your time, Markus
Signed-off-by: Markus Osterhoff linux-kernel@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;
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@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@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;
After a for-loop was replaced by list_for_each_entry, see Commit bbbc7e8502c9 ("ALSA: hda - Allocate hda_pcm objects dynamically"), Commit 751e2216899c ("ALSA: hda: fix possible null dereference"), a possible NULL pointer dereference has been introduced; this patch adds the NULL check on pcm->pcm, while leaving a potentially superfluous check on pcm itself untouched.
Signed-off-by: Markus Osterhoff linux-kernel@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..d1a2cb6 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 || pcm->own_chmap || !hinfo->substreams) continue; elem = hinfo->chmap ? hinfo->chmap : snd_pcm_std_chmaps;
On Mon, 24 Aug 2015 14:11:39 +0200, Markus Osterhoff wrote:
After a for-loop was replaced by list_for_each_entry, see Commit bbbc7e8502c9 ("ALSA: hda - Allocate hda_pcm objects dynamically"), Commit 751e2216899c ("ALSA: hda: fix possible null dereference"), a possible NULL pointer dereference has been introduced; this patch adds the NULL check on pcm->pcm, while leaving a potentially superfluous check on pcm itself untouched.
Signed-off-by: Markus Osterhoff linux-kernel@k-raum.org
The patch couldn't be applied cleanly because the tabs are replaced wrongly with spaces, so I applied it manually now. Please fix your MUA or the way to create a patch at the next time.
thanks,
Takashi
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..d1a2cb6 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 || pcm->own_chmap || !hinfo->substreams) continue; elem = hinfo->chmap ? hinfo->chmap : snd_pcm_std_chmaps;
participants (2)
-
Markus Osterhoff -
Takashi Iwai