| Message ID | 20180308200653.GA47801@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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.
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
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.
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;
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(+)