On 2022/10/03 1:04, Enrico Mioso wrote: > Hello all! > > Is this the expected fix for the issue? > Shouldn't the value be sanitized somehow? > This is intended to be an "honest" question - I am not an experienced kernel nor filesystem programmer, just wondering... Well, there would be two approaches. One is to limit values in u32 range, as attempted by Kari Argillander. The other is to handle values in u64 range, as attempted by me. A crafted filesystem image used by this reproducer has inode->i_size=18446743966335434752 sbi->record_bits=10 tt=18446744073604694080 before calling err = wnd_init(&sbi->mft.bitmap, sb, tt); in ntfs_fill_super(). Then, passing such value to wnd_init() makes nbits=18446744073604694080 bitmap_size(nbits)=2305843009200586760 sb->s_blocksize=4096 sb->s_blocksize_bits=12 wnd->nwnd=562949953418113 which attempts 1023TB memory allocation. If bitmap_size() were truncated to u32, wnd_init() gets nbits=18446744073604694080 bitmap_size(nbits)=4281860104 sb->s_blocksize=4096 sb->s_blocksize_bits=12 wnd->nwnd=1045377 which attempts 2MB memory allocation. Although we need to be careful with u64 overflow inside bitmap_size() and bytes_to_block(), I guess that handling all values as 64bits is a preferred approach. If you want to go with limiting to "u32", please add explanation to Kari's https://syzkaller.appspot.com/text?tag=Patch&x=16c34f22880000 that u32 truncation is needed for avoiding too large memory allocation. A mismatch between function's return value "size_t == u64" and return statement's "u32" casting looks buggy without clear explanation. Well, bitmap_size() should not depend on size_t which can be "u32"? But after all, a patch for "WARNING in ntfs_fill_super" case at https://lkml.kernel.org/r/2405d6f0-3b1a-6405-610f-fd2efab86bdd@xxxxxxxxxxxxxxxxxxx cannot be replaced by Kari's patch, for bitmap_size() is not called in this case. >> filesystem can become wnd->nwnd close to UINT_MAX. Add __GFP_NOWARN in Debug printk() showed that wnd->nwnd was far beyond UINT_MAX due to 64bits. I should update this part if we go with handle "u64" values approach. >> order to avoid too large allocation warning, than exhausting memory by >> using kvcalloc().
