Hello,
syzbot found the following issue on:
HEAD commit: b253435746d9 Merge tag 'xtensa-20220416' of https://github..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=144dd7f4f00000
kernel config: https://syzkaller.appspot.com/x/.config?x=ff9f8140cbb3af7
dashboard link: https://syzkaller.appspot.com/bug?extid=1631f09646bc214d2e76
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
loop3: detected capacity change from 0 to 8189
================================================================================
UBSAN: shift-out-of-bounds in fs/ntfs3/super.c:673:16
shift exponent -192 is negative
CPU: 0 PID: 12945 Comm: syz-executor.3 Not tainted 5.18.0-rc3-syzkaller-00016-gb253435746d9 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
ubsan_epilogue+0xb/0x50 lib/ubsan.c:151
__ubsan_handle_shift_out_of_bounds.cold+0xb1/0x187 lib/ubsan.c:322
ntfs_fill_super.cold+0x2bf/0x549 fs/ntfs/bitmap.c:84
get_tree_bdev+0x440/0x760 fs/super.c:1292
vfs_get_tree+0x89/0x2f0 fs/super.c:1497
do_new_mount fs/namespace.c:3040 [inline]
path_mount+0x1320/0x1fa0 fs/namespace.c:3370
do_mount fs/namespace.c:3383 [inline]
__do_sys_mount fs/namespace.c:3591 [inline]
__se_sys_mount fs/namespace.c:3568 [inline]
__x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f768c28a61a
Code: 48 c7 c2 b8 ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 b8 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f768d3dff88 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000020000200 RCX: 00007f768c28a61a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f768d3dffe0
RBP: 00007f768d3e0020 R08: 00007f768d3e0020 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000206 R12: 0000000020000000
R13: 0000000020000100 R14: 00007f768d3dffe0 R15: 000000002007aa80
</TASK>
================================================================================
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
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 <[email protected]>
Signed-off-by: Tetsuo Handa <[email protected]>
Tested-by: syzbot <[email protected]>
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
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 <[email protected]>
> Signed-off-by: Tetsuo Handa <[email protected]>
> Tested-by: syzbot <[email protected]>
> 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/[email protected]/
but it appears that the NTFS3 maintainer is MIA again. :(
--
~Randy
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 2022/09/23 9:38, Randy Dunlap wrote:
> I slightly prefer the earlier patch:
>
> https://lore.kernel.org/all/[email protected]/
>
> 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 [email protected]
but also [email protected] 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?
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/[email protected]/
--
~Randy
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/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 <[email protected]>
>> Signed-off-by: Tetsuo Handa <[email protected]>
>> Tested-by: syzbot <[email protected]>
>> 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/[email protected]/
>
> 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 Tetsuo,
Thank you very much for providing the feedback!
On Fri, Sep 23, 2022 at 3:26 AM Tetsuo Handa
<[email protected]> wrote:
>
> On 2022/09/23 9:38, Randy Dunlap wrote:
> > I slightly prefer the earlier patch:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > 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 [email protected]
> but also [email protected] 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 [email protected].
> 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.
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 <[email protected]>
>>> Signed-off-by: Tetsuo Handa <[email protected]>
>>> Tested-by: syzbot <[email protected]>
>>> 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/[email protected]/
>>
>> 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 Fri, Sep 23, 2022 at 4:35 PM Tetsuo Handa
<[email protected]> 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 [email protected].
> 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 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 <[email protected]>
> Signed-off-by: Tetsuo Handa <[email protected]>
> Tested-by: syzbot <[email protected]>
> 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.