| Message ID | 20220409012655.9399-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm: Test for "silence" field in struct "pcm_format_data" | expand |
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.
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
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. >
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.
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
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
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
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) {
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(-)