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

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

[Fabio M. De Francesco]

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) {
-- 
2.34.1

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

[Fabio M. De Francesco]

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) {
-- 
2.34.1

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

[Takashi Iwai]

On Sat, 09 Apr 2022 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>

Thanks, applied now.


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

That must be present from the very beginning.
I just add Cc-to-stable for allowing backport to all releases.


thanks,

Takashi

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

[Takashi Iwai]

On Sat, 09 Apr 2022 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>

Thanks, applied now.


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

That must be present from the very beginning.
I just add Cc-to-stable for allowing backport to all releases.


thanks,

Takashi

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

[Eugeniu Rosca]

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.

I haven't fully validated the finding, but it appears to be legit,
since the pointer variable (as opposed to the contents behind the
pointer) is always non-null, hence !pat always evaluating to false.

If the above is true, then the patch likely hasn't introduced any
regression, but also likely hasn't fixed the original KASAN problem.

Or are there alternative views?

BR, Eugeniu.

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

[Eugeniu Rosca]

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.

I haven't fully validated the finding, but it appears to be legit,
since the pointer variable (as opposed to the contents behind the
pointer) is always non-null, hence !pat always evaluating to false.

If the above is true, then the patch likely hasn't introduced any
regression, but also likely hasn't fixed the original KASAN problem.

Or are there alternative views?

BR, Eugeniu.

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

[Takashi Iwai]

On Tue, 14 Jun 2022 11:58:51 +0200,
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.
> 
> I haven't fully validated the finding, but it appears to be legit,
> since the pointer variable (as opposed to the contents behind the
> pointer) is always non-null, hence !pat always evaluating to false.
> 
> If the above is true, then the patch likely hasn't introduced any
> regression, but also likely hasn't fixed the original KASAN problem.
> 
> Or are there alternative views?

Indeed the fix looks bogus, and maybe better to revert.

Looking at the original syzkaller report again, it points rather to
the *write* at the address 1, and it means not the source (silence[])
but the target pointer (data) is invalid; i.e. it's a problem in the
caller side, likely some race between the OSS temporary buffer removal
and other operation.

Thanks for checking this.


Takashi

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

[Takashi Iwai]

On Tue, 14 Jun 2022 11:58:51 +0200,
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.
> 
> I haven't fully validated the finding, but it appears to be legit,
> since the pointer variable (as opposed to the contents behind the
> pointer) is always non-null, hence !pat always evaluating to false.
> 
> If the above is true, then the patch likely hasn't introduced any
> regression, but also likely hasn't fixed the original KASAN problem.
> 
> Or are there alternative views?

Indeed the fix looks bogus, and maybe better to revert.

Looking at the original syzkaller report again, it points rather to
the *write* at the address 1, and it means not the source (silence[])
but the target pointer (data) is invalid; i.e. it's a problem in the
caller side, likely some race between the OSS temporary buffer removal
and other operation.

Thanks for checking this.


Takashi

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

[Fabio M. De Francesco]

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.

I'd better read again how fields of global struct variables are initialized 
:-(

Thanks for this finding,

Fabio

> 
> I haven't fully validated the finding, but it appears to be legit,
> since the pointer variable (as opposed to the contents behind the
> pointer) is always non-null, hence !pat always evaluating to false.
> 
> If the above is true, then the patch likely hasn't introduced any
> regression, but also likely hasn't fixed the original KASAN problem.
> 
> Or are there alternative views?
> 
> BR, Eugeniu.
> 

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

[Fabio M. De Francesco]

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.

I'd better read again how fields of global struct variables are initialized 
:-(

Thanks for this finding,

Fabio

> 
> I haven't fully validated the finding, but it appears to be legit,
> since the pointer variable (as opposed to the contents behind the
> pointer) is always non-null, hence !pat always evaluating to false.
> 
> If the above is true, then the patch likely hasn't introduced any
> regression, but also likely hasn't fixed the original KASAN problem.
> 
> Or are there alternative views?
> 
> BR, Eugeniu.
> 

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

[Eugeniu Rosca]

Dear Takashi, dear Fabio,

Thank you for your prompt feedback.
Please, keep me in the loop in case a revert/fix is submitted to LKML.

BR, Eugeniu.

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

[Eugeniu Rosca]

Dear Takashi, dear Fabio,

Thank you for your prompt feedback.
Please, keep me in the loop in case a revert/fix is submitted to LKML.

BR, Eugeniu.

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

[Takashi Iwai]

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

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

[Takashi Iwai]

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

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

[Fabio M. De Francesco]

On martedì 14 giugno 2022 12:49:38 CEST Takashi Iwai wrote:
> 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...
> 
Does it imply that my argument is correct or my "fix" can't yet catch those 
missing ones?

Besides the question above, I want to notice that we have one more /* FIXME 
*/ entry...

/* FIXME: the following format is not defined properly yet */
	[SNDRV_PCM_FORMAT_SPECIAL] = {
		.le = -1, .signd = -1,
	},

If you want I can get rid of those three entries if you confirm they can 
safely be deleted. In a second patch I can also remove that unnecessary 
check for valid "pat".

Please let me know.

Thanks,

Fabio

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

[Fabio M. De Francesco]

On martedì 14 giugno 2022 12:49:38 CEST Takashi Iwai wrote:
> 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...
> 
Does it imply that my argument is correct or my "fix" can't yet catch those 
missing ones?

Besides the question above, I want to notice that we have one more /* FIXME 
*/ entry...

/* FIXME: the following format is not defined properly yet */
	[SNDRV_PCM_FORMAT_SPECIAL] = {
		.le = -1, .signd = -1,
	},

If you want I can get rid of those three entries if you confirm they can 
safely be deleted. In a second patch I can also remove that unnecessary 
check for valid "pat".

Please let me know.

Thanks,

Fabio

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

[Takashi Iwai]

On Tue, 14 Jun 2022 13:30:13 +0200,
Fabio M. De Francesco wrote:
> 
> On martedì 14 giugno 2022 12:49:38 CEST Takashi Iwai wrote:
> > 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...
> > 
> Does it imply that my argument is correct or my "fix" can't yet catch those 
> missing ones?

Your fix should catch the case with a NULL pat pointer, I guess.
PCM OSS layer allows this format, so this could be hit.
However, whether it's really a fix for the given syzkaller code path,
it's doubtful.  Nevertheless, your check is still worth to keep.

> Besides the question above, I want to notice that we have one more /* FIXME 
> */ entry...
> 
> /* FIXME: the following format is not defined properly yet */
> 	[SNDRV_PCM_FORMAT_SPECIAL] = {
> 		.le = -1, .signd = -1,
> 	},
> 
> If you want I can get rid of those three entries if you confirm they can 
> safely be deleted. In a second patch I can also remove that unnecessary 
> check for valid "pat".

Well, you can't "delete" those entries so easiy.  The formats
themselves are still allowed for user-space, hence every caller needs
to check.  If any, we need to add the check in valid_format() for such
unsupported formats, then drop the rest checks and entries.


thanks,

Takashi

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

[Takashi Iwai]

On Tue, 14 Jun 2022 13:30:13 +0200,
Fabio M. De Francesco wrote:
> 
> On martedì 14 giugno 2022 12:49:38 CEST Takashi Iwai wrote:
> > 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...
> > 
> Does it imply that my argument is correct or my "fix" can't yet catch those 
> missing ones?

Your fix should catch the case with a NULL pat pointer, I guess.
PCM OSS layer allows this format, so this could be hit.
However, whether it's really a fix for the given syzkaller code path,
it's doubtful.  Nevertheless, your check is still worth to keep.

> Besides the question above, I want to notice that we have one more /* FIXME 
> */ entry...
> 
> /* FIXME: the following format is not defined properly yet */
> 	[SNDRV_PCM_FORMAT_SPECIAL] = {
> 		.le = -1, .signd = -1,
> 	},
> 
> If you want I can get rid of those three entries if you confirm they can 
> safely be deleted. In a second patch I can also remove that unnecessary 
> check for valid "pat".

Well, you can't "delete" those entries so easiy.  The formats
themselves are still allowed for user-space, hence every caller needs
to check.  If any, we need to add the check in valid_format() for such
unsupported formats, then drop the rest checks and entries.


thanks,

Takashi