[PATCH] ASoC: soc-core: Add missing NULL check

[PATCH] ASoC: soc-core: Add missing NULL check

[Kees Cook]

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 <pavel@ucw.cz>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(+)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 96c44f6576c9..78ad165ad424 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -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;
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

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

> 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"?

> +++ b/sound/soc/soc-core.c
> @@ -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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

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

> 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"?

> +++ b/sound/soc/soc-core.c
> @@ -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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Kees Cook]

On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:
>
>> 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...)

>
>> +++ b/sound/soc/soc-core.c
>> @@ -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.

No idea! Hopefully Pavel has more details...

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

On Fri 2018-03-09 10:45:16, Kees Cook wrote:
> On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:
> >
> >> 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...)

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.

> >> +++ b/sound/soc/soc-core.c
> >> @@ -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.
> 
> No idea! Hopefully Pavel has more details...

Pavel probably can reproduce it...

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Fri 2018-03-09 12:50:50, Mark Brown wrote:
> On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:
> 
> > 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"?
> 
> > +++ b/sound/soc/soc-core.c
> > @@ -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.

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1086 bytes --]

On Fri 2018-03-09 12:50:50, Mark Brown wrote:
> On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:
> 
> > 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"?
> 
> > +++ b/sound/soc/soc-core.c
> > @@ -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.

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

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Fri, Mar 09, 2018 at 10:45:16AM -0800, Kees Cook wrote:
> On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:

> >> 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?

> >> +     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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1768 bytes --]

On Fri, Mar 09, 2018 at 10:45:16AM -0800, Kees Cook wrote:
> On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Mar 08, 2018 at 12:06:53PM -0800, Kees Cook wrote:

> >> 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?

> >> +     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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: soc-core: Add missing NULL check

[Charles Keepax]

On Fri, Mar 09, 2018 at 08:22:42PM +0000, Mark Brown wrote:
> On Fri, Mar 09, 2018 at 10:45:16AM -0800, Kees Cook wrote:
> > On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> > > 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

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Charles Keepax]

On Fri, Mar 09, 2018 at 08:22:42PM +0000, Mark Brown wrote:
> On Fri, Mar 09, 2018 at 10:45:16AM -0800, Kees Cook wrote:
> > On Fri, Mar 9, 2018 at 4:50 AM, Mark Brown <broonie@kernel.org> wrote:
> > > 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

Re: [alsa-devel] [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Mon, Mar 12, 2018 at 10:31:43AM +0000, Charles Keepax wrote:
> On Fri, Mar 09, 2018 at 08:22:42PM +0000, Mark Brown 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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: soc-core: Add missing NULL check

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 784 bytes --]

On Mon, Mar 12, 2018 at 10:31:43AM +0000, Charles Keepax wrote:
> On Fri, Mar 09, 2018 at 08:22:42PM +0000, Mark Brown 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.

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]