* [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-04-09 1:26 ` Fabio M. De Francesco 0 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-04-09 1:26 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel Cc: Fabio M. De Francesco, syzbot+205eb15961852c2c5974 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-04-09 1:26 ` Fabio M. De Francesco 0 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-04-09 1:26 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel Cc: syzbot+205eb15961852c2c5974, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-04-09 1:26 ` Fabio M. De Francesco @ 2022-04-11 7:30 ` Takashi Iwai -1 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-04-11 7:30 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-04-11 7:30 ` Takashi Iwai 0 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-04-11 7:30 UTC (permalink / raw) To: Fabio M. De Francesco Cc: alsa-devel, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-04-09 1:26 ` Fabio M. De Francesco @ 2022-06-14 9:58 ` Eugeniu Rosca -1 siblings, 0 replies; 18+ messages in thread From: Eugeniu Rosca @ 2022-06-14 9:58 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca, 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 9:58 ` Eugeniu Rosca 0 siblings, 0 replies; 18+ messages in thread From: Eugeniu Rosca @ 2022-06-14 9:58 UTC (permalink / raw) To: Fabio M. De Francesco Cc: alsa-devel, Eugeniu Rosca, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 9:58 ` Eugeniu Rosca @ 2022-06-14 10:27 ` Takashi Iwai -1 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 10:27 UTC (permalink / raw) To: Eugeniu Rosca Cc: Fabio M. De Francesco, Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 10:27 ` Takashi Iwai 0 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 10:27 UTC (permalink / raw) To: Eugeniu Rosca Cc: alsa-devel, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari, Fabio M. De Francesco 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 9:58 ` Eugeniu Rosca @ 2022-06-14 10:43 ` Fabio M. De Francesco -1 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-06-14 10:43 UTC (permalink / raw) To: Eugeniu Rosca Cc: Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca, Eugeniu Rosca 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. > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 10:43 ` Fabio M. De Francesco 0 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-06-14 10:43 UTC (permalink / raw) To: Eugeniu Rosca Cc: alsa-devel, Eugeniu Rosca, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari 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. > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 10:43 ` Fabio M. De Francesco @ 2022-06-14 10:48 ` Eugeniu Rosca -1 siblings, 0 replies; 18+ messages in thread From: Eugeniu Rosca @ 2022-06-14 10:48 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Eugeniu Rosca, Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 10:48 ` Eugeniu Rosca 0 siblings, 0 replies; 18+ messages in thread From: Eugeniu Rosca @ 2022-06-14 10:48 UTC (permalink / raw) To: Fabio M. De Francesco Cc: alsa-devel, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari, 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 10:43 ` Fabio M. De Francesco @ 2022-06-14 10:49 ` Takashi Iwai -1 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 10:49 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Eugeniu Rosca, Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 10:49 ` Takashi Iwai 0 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 10:49 UTC (permalink / raw) To: Fabio M. De Francesco Cc: alsa-devel, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 10:49 ` Takashi Iwai @ 2022-06-14 11:30 ` Fabio M. De Francesco -1 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-06-14 11:30 UTC (permalink / raw) To: Takashi Iwai Cc: Eugeniu Rosca, Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 11:30 ` Fabio M. De Francesco 0 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2022-06-14 11:30 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" 2022-06-14 11:30 ` Fabio M. De Francesco @ 2022-06-14 11:46 ` Takashi Iwai -1 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 11:46 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Eugeniu Rosca, Jaroslav Kysela, Takashi Iwai, Mark Brown, alsa-devel, linux-kernel, syzbot+205eb15961852c2c5974, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" @ 2022-06-14 11:46 ` Takashi Iwai 0 siblings, 0 replies; 18+ messages in thread From: Takashi Iwai @ 2022-06-14 11:46 UTC (permalink / raw) To: Fabio M. De Francesco Cc: alsa-devel, Eugeniu Rosca, linux-kernel, Takashi Iwai, syzbot+205eb15961852c2c5974, Mark Brown, naveenkumar.sunkari, Eugeniu Rosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-06-14 11:47 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-09 1:26 [PATCH] ALSA: pcm: Test for "silence" field in struct "pcm_format_data" Fabio M. De Francesco 2022-04-09 1:26 ` Fabio M. De Francesco 2022-04-11 7:30 ` Takashi Iwai 2022-04-11 7:30 ` Takashi Iwai 2022-06-14 9:58 ` Eugeniu Rosca 2022-06-14 9:58 ` Eugeniu Rosca 2022-06-14 10:27 ` Takashi Iwai 2022-06-14 10:27 ` Takashi Iwai 2022-06-14 10:43 ` Fabio M. De Francesco 2022-06-14 10:43 ` Fabio M. De Francesco 2022-06-14 10:48 ` Eugeniu Rosca 2022-06-14 10:48 ` Eugeniu Rosca 2022-06-14 10:49 ` Takashi Iwai 2022-06-14 10:49 ` Takashi Iwai 2022-06-14 11:30 ` Fabio M. De Francesco 2022-06-14 11:30 ` Fabio M. De Francesco 2022-06-14 11:46 ` Takashi Iwai 2022-06-14 11:46 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.