Skip to contentSkip to conversation

ASoC: soc-core: Add missing NULL check

PATCH In review Kees Cook 4 participants 8 messages Mar 8, 2018 – Mar 12, 2018

Subsystems (2)
Sound ASoC
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 ASoC: soc-core: Add missing NULL check

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

[ 2278.331878] DSS: context saved
[ 2278.820343] Unable to handle kernel NULL pointer dereference at virtual address 00000004
[ 2278.828948] pgd = c36040a2
[ 2278.831787] [00000004] *pgd=876c4831, *pte=00000000, *ppte=00000000
[ 2278.838439] Internal error: Oops: 17 [#1] ARM
[ 2278.843017] Modules linked in:
[ 2278.846221] CPU: 0 PID: 16337 Comm: grep Tainted: G        W 4.16.0-rc4-next-20180308 #71
[ 2278.855529] Hardware name: Nokia RX-51 board
[ 2278.860015] PC is at soc_codec_reg_show+0x8/0x19c
[ 2278.864959] LR is at codec_reg_show+0x28/0x30
Reported-by Pavel Machek <[email protected]>
Signed-off-by Kees Cook <[email protected]>

---
No idea if this is the _right_ fix, but it should stop the crash from unprivileged userspace.

---
 sound/soc/soc-core.c | 3 +++
 1 file changed, 3 insertions(+)
sound/soc/soc-core.c +3 -0
index 96c44f6576c9..78ad165ad424 100644
@@ -137,6 +137,9 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
 	size_t total = 0;
 	loff_t p = 0;
 
+	if (!codec || !codec->driver)
+		return 0;
+
 	wordsize = min_bytes_needed(codec->driver->reg_cache_size) * 2;
 	regsize = codec->driver->reg_word_size * 2;
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
maintainer
#1 lore raw
Quoted 2 lines · Kees Cook

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

I can't parse this, sorry. What is the "sound soc"?

sound/soc/soc-core.c static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, · line 142

Quoted 8 lines

+++ b/sound/soc/soc-core.c

sound/soc/soc-core.c +3 -0
@@ -137,6 +137,9 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
 	size_t total = 0;
 	loff_t p = 0;
 
+	if (!codec || !codec->driver)
+		return 0;
+

How are we managing to create a sysfs file for a CODEC which doesn't have a CODEC struct associated with it? That is obviously nonsensical and suggests we've got some more serious problem going on here - if there's no CODEC those sysfs attributes simply shouldn't be there.

author
#2 lore raw
Quoted 6 lines · Mark Brown
Quoted 2 lines · Kees Cook

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

I can't parse this, sorry. What is the "sound soc"?

SoC's sound component? I'm not sure either. :) I was just sending the patch that I mentioned from the thread where Pavel mentioned this Oops.

Pavel, can you isolate the specific file that is causing the oops? (Maybe this patch should be a WARN() instead of silent return 0, since we still don't want to crash, but it should be considered a bug...)

Quoted 6 lines
Quoted 6 lines

+++ b/sound/soc/soc-core.c

sound/soc/soc-core.c +0 -0
@@ -137,6 +137,9 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
      size_t total = 0;
      loff_t p = 0;
Quoted 8 lines

sound/soc/soc-core.c static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, · line 142

Quoted 3 lines
sound/soc/soc-core.c +3 -0
+     if (!codec || !codec->driver)
+             return 0;
+

How are we managing to create a sysfs file for a CODEC which doesn't have a CODEC struct associated with it? That is obviously nonsensical and suggests we've got some more serious problem going on here - if there's no CODEC those sysfs attributes simply shouldn't be there.

No idea! Hopefully Pavel has more details...

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#3 lore raw
Quoted 8 lines · Kees Cook
Quoted 6 lines · Mark Brown
Quoted 2 lines · Kees Cook

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

I can't parse this, sorry. What is the "sound soc"?

Quoted 7 lines

SoC's sound component? I'm not sure either. :) I was just sending the patch that I mentioned from the thread where Pavel mentioned this Oops.

Pavel, can you isolate the specific file that is causing the oops? (Maybe this patch should be a WARN() instead of silent return 0, since we still don't want to crash, but it should be considered a bug...)

Crash is reproducible on linux-next on Nokia N900. But I seen hang on Nokia N9, with different kernel, that may be related.

And yes, WARN() would be nicer.

Quoted 14 lines
Quoted 5 lines
Quoted 5 lines

+++ b/sound/soc/soc-core.c

sound/soc/soc-core.c +0 -0
@@ -137,6 +137,9 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
      size_t total = 0;
      loff_t p = 0;
Quoted 8 lines

sound/soc/soc-core.c static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, · line 142

Quoted 3 lines
sound/soc/soc-core.c +3 -0
+     if (!codec || !codec->driver)
+             return 0;
+

How are we managing to create a sysfs file for a CODEC which doesn't have a CODEC struct associated with it? That is obviously nonsensical and suggests we've got some more serious problem going on here - if there's no CODEC those sysfs attributes simply shouldn't be there.

Quoted 1 line

No idea! Hopefully Pavel has more details...

Pavel probably can reproduce it...

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#4 lore raw
In reply to Kees Cook
Quoted 11 lines · Kees Cook
Quoted 6 lines · Mark Brown
Quoted 2 lines · Kees Cook

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

I can't parse this, sorry. What is the "sound soc"?

SoC's sound component? I'm not sure either. :) I was just sending the patch that I mentioned from the thread where Pavel mentioned this Oops.

Oh, Pavel's thing. I didn't look at that yet. I'm afraid your description still isn't making much sense to me - I'm guessing that you're just papering over an immediate crack rather than having analyized the situation in any depth?

Quoted 9 lines
Quoted 7 lines

sound/soc/soc-core.c static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, · line 141

Quoted 2 lines
sound/soc/soc-core.c +2 -0
+     if (!codec || !codec->driver)
+             return 0;

How are we managing to create a sysfs file for a CODEC which doesn't have a CODEC struct associated with it? That is obviously nonsensical and suggests we've got some more serious problem going on here - if there's no CODEC those sysfs attributes simply shouldn't be there.

No idea! Hopefully Pavel has more details...

That's where the fix should be, it implies that there's some larger data corruption/confusion problem somewhere else. If we've created the file but left a NULL pointer I'd expect that there is a good chance that there'll be other things that think we've got a CODEC and try to defererence the pointer, it's an assumption that's present throughout the code.

I think I might just remove the file though, it's been non-functional on most systems for a while now as almost all the drivers migrated to regmap and nobody complained so we should be safe. There's still something that ought to be investigated here.

#5 lore raw
Quoted 7 lines · Mark Brown
Quoted 2 lines · Kees Cook
Quoted 1 line · Mark Brown

On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:

I think I might just remove the file though, it's been non-functional on most systems for a while now as almost all the drivers migrated to regmap and nobody complained so we should be safe. There's still something that ought to be investigated here.

Looks like we might already be about to try that since it looks like the componentisation actually removed these files, although I assume inadvertently.

Thanks,
Charles

maintainer
#6 lore raw
Quoted 10 lines · Charles Keepax
Quoted 4 lines · Mark Brown

I think I might just remove the file though, it's been non-functional on most systems for a while now as almost all the drivers migrated to regmap and nobody complained so we should be safe. There's still something that ought to be investigated here.

Looks like we might already be about to try that since it looks like the componentisation actually removed these files, although I assume inadvertently.

No, that's an intended effect. This only exists for CODEC drivers and even then it's only ones that use the legacy ASoC I/O code that really ever got anything from this. Anything that's well maintained should be using regmap.

#7 lore raw
In reply to Mark Brown
Quoted 12 lines · Mark Brown
Quoted 2 lines · Kees Cook

If a codec is not attached to the sound soc, a NULL deref is possible as a regular user in /sys.

I can't parse this, sorry. What is the "sound soc"?

Quoted 5 lines

+++ b/sound/soc/soc-core.c

sound/soc/soc-core.c +0 -0
@@ -137,6 +137,9 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf,
 	size_t total = 0;
 	loff_t p = 0;
Quoted 8 lines

sound/soc/soc-core.c static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, · line 142

Quoted 3 lines
sound/soc/soc-core.c +3 -0
+	if (!codec || !codec->driver)
+		return 0;
+

How are we managing to create a sysfs file for a CODEC which doesn't have a CODEC struct associated with it? That is obviously nonsensical and suggests we've got some more serious problem going on here - if there's no CODEC those sysfs attributes simply shouldn't be there.

Look for "linux-next on n900: oops in codec_reg_show() when grepping sysfs" ... should be in your inbox.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html