ASoC: soc-core: Add missing NULL check
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
---
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
@@ -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
Mark Brown
maintainerQuoted 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.csound/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.
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.csound/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
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.csound/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
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.csound/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
Mark Brown
maintainerQuoted 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.
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
Mark Brown
maintainerQuoted 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.