Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data"

[Takashi Iwai <tiwai@suse.de>]

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@google.com/
> > > 
> > > Reported-and-tested-by: 
> syzbot+205eb15961852c2c5974@syzkaller.appspotmail.com
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.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