linux-kernel - Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y1xzplj1.wl-tiwai@suse.de>
Date:   Tue, 14 Jun 2022 12:49:38 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Eugeniu Rosca <erosca@...adit-jv.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, Mark Brown <broonie@...nel.org>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        syzbot+205eb15961852c2c5974@...kaller.appspotmail.com,
        naveenkumar.sunkari@...bosch.com,
        Eugeniu Rosca <roscaeugeniu@...il.com>
Subject: Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data"

On Tue, 14 Jun 2022 12:43:16 +0200,
Fabio M. De Francesco wrote:
> 
> On martedì 14 giugno 2022 11:58:51 CEST Eugeniu Rosca wrote:
> > Hello Fabio, hello All,
> > 
> > On Sa, Apr 09, 2022 at 03:26:55 +0200, Fabio M. De Francesco wrote:
> > > Syzbot reports "KASAN: null-ptr-deref Write in
> > > snd_pcm_format_set_silence".[1]
> > > 
> > > It is due to missing validation of the "silence" field of struct
> > > "pcm_format_data" in "pcm_formats" array.
> > > 
> > > Add a test for valid "pat" and, if it is not so, return -EINVAL.
> > > 
> > > [1] https://lore.kernel.org/lkml/
> 000000000000d188ef05dc2c7279@...gle.com/
> > > 
> > > Reported-and-tested-by: 
> syzbot+205eb15961852c2c5974@...kaller.appspotmail.com
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > > ---
> > > 
> > > I wasn't able to figure out the commit for the "Fixes:" tag. If this 
> patch
> > > is good, can someone please help with providing this missing 
> information?
> > > 
> > >  sound/core/pcm_misc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
> > > index 4866aed97aac..5588b6a1ee8b 100644
> > > --- a/sound/core/pcm_misc.c
> > > +++ b/sound/core/pcm_misc.c
> > > @@ -433,7 +433,7 @@ int snd_pcm_format_set_silence(snd_pcm_format_t 
> format, void *data, unsigned int
> > >  		return 0;
> > >  	width = pcm_formats[(INT)format].phys; /* physical width */
> > >  	pat = pcm_formats[(INT)format].silence;
> > > -	if (! width)
> > > +	if (!width || !pat)
> > >  		return -EINVAL;
> > >  	/* signed or 1 byte data */
> > >  	if (pcm_formats[(INT)format].signd == 1 || width <= 8) {
> > 
> > JFYI, PVS-Studio 7.19 reports:
> > 
> > sound/core/pcm_misc.c	409	warn	V560 A part of 
> conditional expression is always false: !pat.
> 
> Sorry, I assumed (wrongly!) that when we have
> 
> static const struct pcm_format_data 
> pcm_formats[(INT)SNDRV_PCM_FORMAT_LAST+1] = {
> 	[SNDRV_PCM_FORMAT_S8] = {
> 		.width = 8, .phys = 8, .le = -1, .signd = 1,
> 		.silence = {},
> 	},
> 	[snip]
> 	/* FIXME: the following two formats are not defined properly yet 
> */
> 	[SNDRV_PCM_FORMAT_MPEG] = {
> 		.le = -1, .signd = -1,
> 	},
> 	[SNDRV_PCM_FORMAT_GSM] = {
> 		.le = -1, .signd = -1,
> 	},
> 
> pointer "silence", and then "pat", must be NULL.

Oh right, those are missing ones.  I haven't realized that those
formats are allowed by PCM OSS layer.

Practically seen, those formats have never been used in reality, and
we may consider dropping them completely to plug such holes...


Takashi