[syzbot] [ntfs3?] WARNING in ntfs_load_attr_list

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: a5541c0811a0 Merge branch 'for-next/core' into for-kernelci
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=147b73ac480000
kernel config: https://syzkaller.appspot.com/x/.config?x=cbd4e584773e9397
dashboard link: https://syzkaller.appspot.com/bug?extid=89dbb3a789a5b9711793
compiler: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm64
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12a9b338480000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=12c4bcff880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/4b7702208fb9/disk-a5541c08.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/9ec0153ec051/vmlinux-a5541c08.xz
kernel image: https://storage.googleapis.com/syzbot-assets/6f8725ad290a/Image-a5541c08.gz.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/e51299d60a31/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+89dbb3...@syzkaller.appspotmail.com

loop0: detected capacity change from 0 to 4096
ntfs3: loop0: Different NTFS' sector size (1024) and media sector size (512)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 3074 at mm/page_alloc.c:5534 __alloc_pages+0x150/0x1fc mm/page_alloc.c:5534
Modules linked in:
CPU: 0 PID: 3074 Comm: syz-executor509 Not tainted 6.1.0-rc8-syzkaller-33330-ga5541c0811a0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : __alloc_pages+0x150/0x1fc mm/page_alloc.c:5534
lr : __alloc_pages_node include/linux/gfp.h:237 [inline]
lr : alloc_pages_node include/linux/gfp.h:260 [inline]
lr : __kmalloc_large_node+0xb4/0x1dc mm/slab_common.c:1096
sp : ffff80000ff138b0
x29: ffff80000ff138f0 x28: 0000000000000001 x27: ffff0000c62ca900
x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
x23: ffff0000ca9316d0 x22: 0000000000000004 x21: 0000000000040c40
x20: 0000000000000000 x19: 0000000000000022 x18: 00000000000000c0
x17: ffff80000dda8198 x16: ffff80000dbe6158 x15: ffff0000c9ae1a40
x14: 0000000000000000 x13: 00000000ffffffff x12: 000000020000000f
x11: 00000000f0000006 x10: 0000000000000040 x9 : 0000000000000001
x8 : ffff80000d95e000 x7 : 0000000000000000 x6 : 000000000000003f
x5 : ffff0000c9bc788c x4 : 0000000000000000 x3 : 0000000000000000
x2 : 0000000000000000 x1 : 0000000000000022 x0 : 0000000000040c40
Call trace:
__alloc_pages+0x150/0x1fc mm/page_alloc.c:5534
__alloc_pages_node include/linux/gfp.h:237 [inline]
alloc_pages_node include/linux/gfp.h:260 [inline]
__kmalloc_large_node+0xb4/0x1dc mm/slab_common.c:1096
__do_kmalloc_node mm/slab_common.c:943 [inline]
__kmalloc+0x104/0x140 mm/slab_common.c:968
kmalloc include/linux/slab.h:558 [inline]
ntfs_load_attr_list+0x124/0x1d0 fs/ntfs3/attrlist.c:78
ntfs_read_mft fs/ntfs3/inode.c:162 [inline]
ntfs_iget5+0x59c/0x138c fs/ntfs3/inode.c:501
ntfs_loadlog_and_replay+0x98/0x1ec fs/ntfs3/fsntfs.c:272
ntfs_fill_super+0xc10/0x14a4 fs/ntfs3/super.c:1018
get_tree_bdev+0x1e8/0x2a0 fs/super.c:1324
ntfs_fs_get_tree+0x28/0x38 fs/ntfs3/super.c:1358
vfs_get_tree+0x40/0x140 fs/super.c:1531
do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
path_mount+0x358/0x890 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]
__arm64_sys_mount+0x2c4/0x3c4 fs/namespace.c:3568
__invoke_syscall arch/arm64/kernel/syscall.c:38 [inline]
invoke_syscall arch/arm64/kernel/syscall.c:52 [inline]
el0_svc_common+0x138/0x220 arch/arm64/kernel/syscall.c:142
do_el0_svc+0x48/0x140 arch/arm64/kernel/syscall.c:197
el0_svc+0x58/0x150 arch/arm64/kernel/entry-common.c:637
el0t_64_sync_handler+0x84/0xf0 arch/arm64/kernel/entry-common.c:655
el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:584
irq event stamp: 23612
hardirqs last enabled at (23611): [<ffff80000852de54>] ___slab_alloc+0x794/0x91c mm/slub.c:3132
hardirqs last disabled at (23612): [<ffff80000c084084>] el1_dbg+0x24/0x80 arch/arm64/kernel/entry-common.c:405
softirqs last enabled at (21864): [<ffff8000080102e4>] _stext+0x2e4/0x37c
softirqs last disabled at (21721): [<ffff800008017c88>] ____do_softirq+0x14/0x20 arch/arm64/kernel/irq.c:80
---[ end trace 0000000000000000 ]---
ntfs3: loop0: Failed to load $MFT.


---
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 syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Tetsuo Handa]

syzbot is reporting too large allocation at ntfs_load_attr_list() [1], for
a crafted filesystem can have huge data_size.

Link: https://syzkaller.appspot.com/bug?extid=89dbb3a789a5b9711793 [1]
Reported-by: syzbot <syzbot+89dbb3...@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
---
fs/ntfs3/attrlist.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c
index c0c6bcbc8c05..81c22df27c72 100644
--- a/fs/ntfs3/attrlist.c
+++ b/fs/ntfs3/attrlist.c
@@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)

if (!attr->non_res) {
lsize = le32_to_cpu(attr->res.data_size);
- le = kmalloc(al_aligned(lsize), GFP_NOFS);
+ le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
if (!le) {
err = -ENOMEM;
goto out;
@@ -80,7 +80,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
if (err < 0)
goto out;

- le = kmalloc(al_aligned(lsize), GFP_NOFS);
+ le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
if (!le) {
err = -ENOMEM;
goto out;
--
2.34.1

[Michal Hocko]

[this has just hit my filters, I am not really familiar with the code
itself]

On Mon 02-01-23 23:53:40, Tetsuo Handa wrote:
> syzbot is reporting too large allocation at ntfs_load_attr_list() [1], for
> a crafted filesystem can have huge data_size.
>
> Link: https://syzkaller.appspot.com/bug?extid=89dbb3a789a5b9711793 [1]
> Reported-by: syzbot <syzbot+89dbb3...@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin...@I-love.SAKURA.ne.jp>
> ---
> fs/ntfs3/attrlist.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c
> index c0c6bcbc8c05..81c22df27c72 100644
> --- a/fs/ntfs3/attrlist.c
> +++ b/fs/ntfs3/attrlist.c
> @@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
>
> if (!attr->non_res) {
> lsize = le32_to_cpu(attr->res.data_size);
> - le = kmalloc(al_aligned(lsize), GFP_NOFS);
> + le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);

This looks like a bad idea in general. The allocator merely says that
something is wrong and you are silencing that. The calling code should
check the size for reasonable range and if larger size. Moreover, if
lsize can be really more than PAGE_SIZE this should be kvmalloc instead.
Ditto for the the other case.

> if (!le) {
> err = -ENOMEM;
> goto out;
> @@ -80,7 +80,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
> if (err < 0)
> goto out;
>
> - le = kmalloc(al_aligned(lsize), GFP_NOFS);
> + le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
> if (!le) {
> err = -ENOMEM;
> goto out;
> --
> 2.34.1
>

--
Michal Hocko
SUSE Labs

[Tetsuo Handa]

On 2023/01/03 5:19, Michal Hocko wrote:
>> @@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
>>
>> if (!attr->non_res) {
>> lsize = le32_to_cpu(attr->res.data_size);
>> - le = kmalloc(al_aligned(lsize), GFP_NOFS);
>> + le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
>
> This looks like a bad idea in general. The allocator merely says that
> something is wrong and you are silencing that. The calling code should
> check the size for reasonable range and if larger size. Moreover, if
> lsize can be really more than PAGE_SIZE this should be kvmalloc instead.

There are already similar commits.

commit 0d0f659bf713 ("fs/ntfs3: Use __GFP_NOWARN allocation at wnd_init()")
commit 59bfd7a483da ("fs/ntfs3: Use __GFP_NOWARN allocation at ntfs_fill_super()")

Is KMALLOC_MAX_SIZE intended to be used by callers like

https://linux.googlesource.com/linux/kernel/git/torvalds/linux/+/a5a1e1f249db4e0a35d3deca0b9916b11cc1f02b%5E!

? I think that, unless there is a known upper limit defined by specification,
checking for overflow and silence like

https://lkml.kernel.org/r/6d878e01-6c2f-8766...@I-love.SAKURA.ne.jp

is fine. These input are random values which do not need to succeed by using kvmalloc().

[Michal Hocko]

On Tue 03-01-23 09:49:22, Tetsuo Handa wrote:
> On 2023/01/03 5:19, Michal Hocko wrote:
> >> @@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
> >>
> >> if (!attr->non_res) {
> >> lsize = le32_to_cpu(attr->res.data_size);
> >> - le = kmalloc(al_aligned(lsize), GFP_NOFS);
> >> + le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
> >
> > This looks like a bad idea in general. The allocator merely says that
> > something is wrong and you are silencing that. The calling code should
> > check the size for reasonable range and if larger size. Moreover, if
> > lsize can be really more than PAGE_SIZE this should be kvmalloc instead.
>
> There are already similar commits.
>
> commit 0d0f659bf713 ("fs/ntfs3: Use __GFP_NOWARN allocation at wnd_init()")
> commit 59bfd7a483da ("fs/ntfs3: Use __GFP_NOWARN allocation at ntfs_fill_super()")

Bad examples to follow.

> Is KMALLOC_MAX_SIZE intended to be used by callers like
>
> https://linux.googlesource.com/linux/kernel/git/torvalds/linux/+/a5a1e1f249db4e0a35d3deca0b9916b11cc1f02b%5E!
>
> ?

Nope, this doesn't look right either. This all is about inhibiting the
warning much more than actually fixing the underlying problem which
would be either check against a _specification_ based or _reasonable_
expectation based range or using kvmalloc instead if the range is not
well defined.

> I think that, unless there is a known upper limit defined by specification,
> checking for overflow and silence like
>
> https://lkml.kernel.org/r/6d878e01-6c2f-8766...@I-love.SAKURA.ne.jp
>
> is fine. These input are random values which do not need to succeed by using kvmalloc().

How can you tell the value is just a random noise or a relevant value
that people would actually want to succeede? Answer to that question
gives you a hint on how to address the issue.

[Michal Hocko]

On Tue 03-01-23 09:01:34, Michal Hocko wrote:
> On Tue 03-01-23 09:49:22, Tetsuo Handa wrote:
> > On 2023/01/03 5:19, Michal Hocko wrote:
> > >> @@ -52,7 +52,7 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct ATTRIB *attr)
> > >>
> > >> if (!attr->non_res) {
> > >> lsize = le32_to_cpu(attr->res.data_size);
> > >> - le = kmalloc(al_aligned(lsize), GFP_NOFS);
> > >> + le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
> > >
> > > This looks like a bad idea in general. The allocator merely says that
> > > something is wrong and you are silencing that. The calling code should
> > > check the size for reasonable range and if larger size. Moreover, if
> > > lsize can be really more than PAGE_SIZE this should be kvmalloc instead.
> >
> > There are already similar commits.
> >
> > commit 0d0f659bf713 ("fs/ntfs3: Use __GFP_NOWARN allocation at wnd_init()")
> > commit 59bfd7a483da ("fs/ntfs3: Use __GFP_NOWARN allocation at ntfs_fill_super()")
>
> Bad examples to follow.
>
> > Is KMALLOC_MAX_SIZE intended to be used by callers like
> >
> > https://linux.googlesource.com/linux/kernel/git/torvalds/linux/+/a5a1e1f249db4e0a35d3deca0b9916b11cc1f02b%5E!
> >
> > ?
>
> Nope, this doesn't look right either. This all is about inhibiting the
> warning much more than actually fixing the underlying problem which
> would be either check against a _specification_ based or _reasonable_
> expectation based range or using kvmalloc instead if the range is not
> well defined.

Let me clarify some more because there are two things happening here.

kvmalloc (or its variants) should be used whenever there is a risk the
allocation request size is large (>>PAGE_SIZE) sounds like reasonable
rule of thumb because those allocations shouldn't put an additional
pressure on a fragmented system.

Any user input, and that applies to potentially crafted fs images,
should be checked for runaway values. If there is specification in
place then this is no brainer. If the value is not well specified then
there should be reasonable defensive checking done. KMALLOC_MAX_SIZE
doesn't sound like a good fit for the check as that is an internal
implementation specific constant for a particular memory allocator.
vmalloc allows much larger allocations and sometime that is a reasonable
thing to allow. So those checks should be domain specific.

[Tetsuo Handa]

Konstantin, what is the expected max size?
Can this allocation for legitimate filesystem become large enough to try vmalloc() ?