syzbot is reporting shift-out-of-bounds in true_sectors_per_clst() [1], for
commit a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
did not address that (0 - boot->sectors_per_clusters) < 0 because "u8" was
chosen for type of boot->sectors_per_clusters because 0x80 needs to be
positive in order to support 64K clusters. Use "s8" cast in order to make
sure that (0 - (s8) boot->sectors_per_clusters) > 0.
Link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 [1]
Reported-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
Fixes: a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 47012c9bf505..c7ffd21fb255 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -672,7 +672,7 @@ static u32 true_sectors_per_clst(const struct NTFS_BOOT *boot)
if (boot->sectors_per_clusters <= 0x80)
return boot->sectors_per_clusters;
if (boot->sectors_per_clusters >= 0xf4) /* limit shift to 2MB max */
- return 1U << (0 - boot->sectors_per_clusters);
+ return 1U << (0 - (s8) boot->sectors_per_clusters);
return -EINVAL;
}
--
2.18.4
On 9/20/22 18:59, Tetsuo Handa wrote:
> syzbot is reporting shift-out-of-bounds in true_sectors_per_clst() [1], for
> commit a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
> did not address that (0 - boot->sectors_per_clusters) < 0 because "u8" was
> chosen for type of boot->sectors_per_clusters because 0x80 needs to be
> positive in order to support 64K clusters. Use "s8" cast in order to make
> sure that (0 - (s8) boot->sectors_per_clusters) > 0.
>
> Link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 [1]
> Reported-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
> Fixes: a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 47012c9bf505..c7ffd21fb255 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -672,7 +672,7 @@ static u32 true_sectors_per_clst(const struct NTFS_BOOT *boot)
> if (boot->sectors_per_clusters <= 0x80)
> return boot->sectors_per_clusters;
> if (boot->sectors_per_clusters >= 0xf4) /* limit shift to 2MB max */
> - return 1U << (0 - boot->sectors_per_clusters);
> + return 1U << (0 - (s8) boot->sectors_per_clusters);
> return -EINVAL;
> }
>
Hello
Thanks for patch, but there was already a similar patch by Shigeru Yoshida,
so I chose it.
Sorry about that, thanks again for your work.
Hi,
On 9/20/22 08:59, Tetsuo Handa wrote:
> syzbot is reporting shift-out-of-bounds in true_sectors_per_clst() [1], for
> commit a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
> did not address that (0 - boot->sectors_per_clusters) < 0 because "u8" was
> chosen for type of boot->sectors_per_clusters because 0x80 needs to be
> positive in order to support 64K clusters. Use "s8" cast in order to make
> sure that (0 - (s8) boot->sectors_per_clusters) > 0.
>
> Link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 [1]
> Reported-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
> Fixes: a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 47012c9bf505..c7ffd21fb255 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -672,7 +672,7 @@ static u32 true_sectors_per_clst(const struct NTFS_BOOT *boot)
> if (boot->sectors_per_clusters <= 0x80)
> return boot->sectors_per_clusters;
> if (boot->sectors_per_clusters >= 0xf4) /* limit shift to 2MB max */
> - return 1U << (0 - boot->sectors_per_clusters);
> + return 1U << (0 - (s8) boot->sectors_per_clusters);
> return -EINVAL;
> }
>
I slightly prefer the earlier patch:
https://lore.kernel.org/all/20220823144625.1627736-1-syoshida@redhat.com/
but it appears that the NTFS3 maintainer is MIA again. :(
--
~Randy
On 9/23/22 03:38, Randy Dunlap wrote:
> Hi,
>
> On 9/20/22 08:59, Tetsuo Handa wrote:
>> syzbot is reporting shift-out-of-bounds in true_sectors_per_clst() [1], for
>> commit a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>> did not address that (0 - boot->sectors_per_clusters) < 0 because "u8" was
>> chosen for type of boot->sectors_per_clusters because 0x80 needs to be
>> positive in order to support 64K clusters. Use "s8" cast in order to make
>> sure that (0 - (s8) boot->sectors_per_clusters) > 0.
>>
>> Link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 [1]
>> Reported-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Tested-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
>> Fixes: a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>>
>> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
>> index 47012c9bf505..c7ffd21fb255 100644
>> --- a/fs/ntfs3/super.c
>> +++ b/fs/ntfs3/super.c
>> @@ -672,7 +672,7 @@ static u32 true_sectors_per_clst(const struct NTFS_BOOT *boot)
>> if (boot->sectors_per_clusters <= 0x80)
>> return boot->sectors_per_clusters;
>> if (boot->sectors_per_clusters >= 0xf4) /* limit shift to 2MB max */
>> - return 1U << (0 - boot->sectors_per_clusters);
>> + return 1U << (0 - (s8) boot->sectors_per_clusters);
>> return -EINVAL;
>> }
>>
>
> I slightly prefer the earlier patch:
>
> https://lore.kernel.org/all/20220823144625.1627736-1-syoshida@redhat.com/
>
> but it appears that the NTFS3 maintainer is MIA again. :(
>
Hello
I've sent patches on 12.09, so I'm not MIA.
I plan to look at patches next week, there are quite a lot of them.
Hi,
On 9/23/22 04:58, Konstantin Komarov wrote:
>
>
> On 9/23/22 03:38, Randy Dunlap wrote:
>> Hi,
>>
>> On 9/20/22 08:59, Tetsuo Handa wrote:
>>> syzbot is reporting shift-out-of-bounds in true_sectors_per_clst() [1], for
>>> commit a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>>> did not address that (0 - boot->sectors_per_clusters) < 0 because "u8" was
>>> chosen for type of boot->sectors_per_clusters because 0x80 needs to be
>>> positive in order to support 64K clusters. Use "s8" cast in order to make
>>> sure that (0 - (s8) boot->sectors_per_clusters) > 0.
>>>
>>> Link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 [1]
>>> Reported-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Tested-by: syzbot <syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com>
>>> Fixes: a3b774342fa752a5 ("fs/ntfs3: validate BOOT sectors_per_clusters")
>>>
>>> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
>>> index 47012c9bf505..c7ffd21fb255 100644
>>> --- a/fs/ntfs3/super.c
>>> +++ b/fs/ntfs3/super.c
>>> @@ -672,7 +672,7 @@ static u32 true_sectors_per_clst(const struct NTFS_BOOT *boot)
>>> if (boot->sectors_per_clusters <= 0x80)
>>> return boot->sectors_per_clusters;
>>> if (boot->sectors_per_clusters >= 0xf4) /* limit shift to 2MB max */
>>> - return 1U << (0 - boot->sectors_per_clusters);
>>> + return 1U << (0 - (s8) boot->sectors_per_clusters);
>>> return -EINVAL;
>>> }
>>>
>>
>> I slightly prefer the earlier patch:
>>
>> https://lore.kernel.org/all/20220823144625.1627736-1-syoshida@redhat.com/
>>
>> but it appears that the NTFS3 maintainer is MIA again. :(
>>
>
> Hello
>
> I've sent patches on 12.09, so I'm not MIA.
> I plan to look at patches next week, there are quite a lot of them.
OK, good news. Thanks.
--
~Randy
On 2022/09/23 9:38, Randy Dunlap wrote: > I slightly prefer the earlier patch: > > https://lore.kernel.org/all/20220823144625.1627736-1-syoshida@redhat.com/ > > but it appears that the NTFS3 maintainer is MIA again. :( > Shigeru Yoshida posted a patch against https://syzkaller.appspot.com/bug?extid=35b87c668935bb55e666 and I posted a patch against https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 . We didn't realize that these are the same problem. It seems that sending to not only syzbot+XXXXXXXXXXXXXXXXXXXX@syzkaller.appspotmail.com but also syzkaller-bugs@googlegroups.com is required for proposed patches to be recorded (in order to avoid duplicated works) into a page linked from "Status:" in each bug page. Since https://syzkaller.appspot.com/upstream does not have a column for tracking intermediate status between "Open" and "Fixed" (e.g. cause identified, patch proposed) because it can take long time until patches are accepted into one of git trees syzbot is tracking, we need to utilize "Last activity" in the list page and a page linked from "Status:" in each bug page. Time to boost priority for implementing user-supplied comment column (e.g. "#syz memo:" command) to each bug?
Hi Tetsuo, Thank you very much for providing the feedback! On Fri, Sep 23, 2022 at 3:26 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/09/23 9:38, Randy Dunlap wrote: > > I slightly prefer the earlier patch: > > > > https://lore.kernel.org/all/20220823144625.1627736-1-syoshida@redhat.com/ > > > > but it appears that the NTFS3 maintainer is MIA again. :( > > > > Shigeru Yoshida posted a patch against https://syzkaller.appspot.com/bug?extid=35b87c668935bb55e666 > and I posted a patch against https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76 . > We didn't realize that these are the same problem. > > It seems that sending to not only syzbot+XXXXXXXXXXXXXXXXXXXX@syzkaller.appspotmail.com > but also syzkaller-bugs@googlegroups.com is required for proposed patches to be recorded > (in order to avoid duplicated works) into a page linked from "Status:" in each bug page. We do have plans to start inspecting LKML messages for the patches that mention syzbot-reported bugs. It will be possible then to display them all on the bug page and somehow mark bugs with a PATCH sent on the list. > > Since https://syzkaller.appspot.com/upstream does not have a column for tracking intermediate > status between "Open" and "Fixed" (e.g. cause identified, patch proposed) because it can take > long time until patches are accepted into one of git trees syzbot is tracking, we need to > utilize "Last activity" in the list page and a page linked from "Status:" in each bug page. > Time to boost priority for implementing user-supplied comment column (e.g. "#syz memo:" command) > to each bug? And then syzbot should just display all such received comments on the bug's web page, right? -- Best Regards, Aleksandr > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/423b1fa6-10fa-3ff9-52bc-1262643c62d9%40I-love.SAKURA.ne.jp.
On 2022/09/23 20:58, Aleksandr Nogikh wrote: > We do have plans to start inspecting LKML messages for the patches > that mention syzbot-reported bugs. It will be possible then to display > them all on the bug page and somehow mark bugs with a PATCH sent on > the list. I interpret it as an attempt to automatically show "Patch proposed" state. But since not all patches have Reported-by: tag, and/or a proposed patch with Reported-by: tag might be withdrawn via review, that state should be also manually changeable. > And then syzbot should just display all such received comments on the > bug's web page, right? Whether "all comments" or "last comment" needs some decision. It might be a few words indicating culprit subsystem (probably "last" should overwrite), it might be memo describing how far debugging went (probably "all" is helpful), it might be some URL where discussions/patches are (probably "all" is helpful), it might be trying to show or hide "Patch proposed" state (probably "last" should overwrite). By the way, a possible improvement on "Patch testing requests:" table. Although the "Patch" link showing diff output after applying proposed patch is OK, I'd like to also see a link to original "#syz test:" mail, for the intent of diff (which would be in patch description part if it was a formal patch) is dropped from diff output in the "Patch" link. For example, https://syzkaller.appspot.com/bug?extid=9ca7a12fd736d93e0232 was forgotten for 1000 days after 7 patch testing requests. I can't easily find the intent of each diff (e.g. just debug printk() or proper fix). It seems the last one was about to formal submit, but I can't find why it is not yet applied.
On Fri, Sep 23, 2022 at 4:35 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2022/09/23 20:58, Aleksandr Nogikh wrote: > > We do have plans to start inspecting LKML messages for the patches > > that mention syzbot-reported bugs. It will be possible then to display > > them all on the bug page and somehow mark bugs with a PATCH sent on > > the list. > > I interpret it as an attempt to automatically show "Patch proposed" state. > But since not all patches have Reported-by: tag, and/or a proposed patch > with Reported-by: tag might be withdrawn via review, that state should be > also manually changeable. Yes, it is meant to be manually changeable. To be honest, I'm a little bit worried about making the syzbot communication protocol more and more complex - e.g. how will other developers figure out that such a feature exists at all.. Though, there are anyway no other options than to extend the protocol. > > > And then syzbot should just display all such received comments on the > > bug's web page, right? > > Whether "all comments" or "last comment" needs some decision. It might be a few words > indicating culprit subsystem (probably "last" should overwrite), it might be memo > describing how far debugging went (probably "all" is helpful), it might be some > URL where discussions/patches are (probably "all" is helpful), it might be trying to > show or hide "Patch proposed" state (probably "last" should overwrite). > It seems that even displaying all patch sending attempts (regardless of their status) should be already very helpful in preventing the situations like you described earlier. E.g. it's very likely that syzbot won't be promptly notified about withdrawn patches, so it's anyway necessary to look at all previous attempts. > > > By the way, a possible improvement on "Patch testing requests:" table. > Although the "Patch" link showing diff output after applying proposed patch is OK, > I'd like to also see a link to original "#syz test:" mail, for the intent of diff > (which would be in patch description part if it was a formal patch) is dropped from > diff output in the "Patch" link. Interesting! I created an issue to keep track of this: https://github.com/google/syzkaller/issues/3392 The presence of the link will, though, depend on whether the user did Cc some public mailing lists while making the patch testing request. > > For example, https://syzkaller.appspot.com/bug?extid=9ca7a12fd736d93e0232 was forgotten > for 1000 days after 7 patch testing requests. I can't easily find the intent of each diff > (e.g. just debug printk() or proper fix). It seems the last one was about to formal submit, > but I can't find why it is not yet applied. Btw there was recently deployed an old repro retesting feature that retests old reproducers and obsoletes bugs if all of them are no longer working. It has already closed > 150 bugs this way (more to come) and in quite a lot of such closed bugs I see a patch testing request from some developer that was done several months or even several years ago. And syzbot was not notified about these fixes. So yes, the presence of a patch testing request can be a strong indicator that the bug is already fixed and syzbot just doesn't know about that. > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/ea7c00c1-07d7-c23e-80f0-0693016e9731%40I-love.SAKURA.ne.jp.
> > By the way, a possible improvement on "Patch testing requests:" table. > > Although the "Patch" link showing diff output after applying proposed patch is OK, > > I'd like to also see a link to original "#syz test:" mail, for the intent of diff > > (which would be in patch description part if it was a formal patch) is dropped from > > diff output in the "Patch" link. > > Interesting! > I created an issue to keep track of this: > https://github.com/google/syzkaller/issues/3392 > The presence of the link will, though, depend on whether the user did > Cc some public mailing lists while making the patch testing request. Upd from Dmitry: https://github.com/google/syzkaller/issues/3392#issuecomment-1256952263 We actually do provide these links, but they're rarely present for the reason I mentioned above.
On Thu, 2022-09-22 at 17:38 -0700, Randy Dunlap wrote: > it appears that the NTFS3 maintainer is MIA again. :( (I've no affiliation with the NTFS3 maintainer or paragon) Perhaps the expectation of prioritized immediate turnaround is unrealistic. It's free. Be patient.
On Thu, Sep 22, 2022 at 05:47:51PM -0700, Joe Perches wrote: > On Thu, 2022-09-22 at 17:38 -0700, Randy Dunlap wrote: > > it appears that the NTFS3 maintainer is MIA again. :( > > (I've no affiliation with the NTFS3 maintainer or paragon) > > Perhaps the expectation of prioritized immediate turnaround is unrealistic. > > It's free. Be patient. It's not just Randy. There are a bunch of fixes which are waiting to be merged... I sometimes look at syzkaller patches but it's like with ntfs3 why even bother because the maintainer is gone? It's not going in anyway. regards, dan carpenter
On 9/22/22 17:47, Joe Perches wrote: > On Thu, 2022-09-22 at 17:38 -0700, Randy Dunlap wrote: >> it appears that the NTFS3 maintainer is MIA again. :( > > (I've no affiliation with the NTFS3 maintainer or paragon) > > Perhaps the expectation of prioritized immediate turnaround is unrealistic. > > It's free. Be patient. > Yes Joe, I get that. https://lore.kernel.org/all/da20d32b-5185-f40b-48b8-2986922d8b25@stargateuniverse.net/ -- ~Randy
© 2016 - 2026 Red Hat, Inc.