[syzbot] [squashfs?] KMSAN: uninit-value in pick_link

[syzbot] [squashfs?] KMSAN: uninit-value in pick_link

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    2f8c4f506285 Merge tag 'auxdisplay-for-v6.11-tag1' of git:..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=145d019d980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=ea3a063e5f96c3d6
dashboard link: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1629a655980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16bfb899980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/ed9f828b1910/disk-2f8c4f50.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/b8bdff998eb1/vmlinux-2f8c4f50.xz
kernel image: https://storage.googleapis.com/syzbot-assets/41b7030717aa/bzImage-2f8c4f50.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/6b20d8f48921/mount_0.gz

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

loop0: detected capacity change from 0 to 8
=====================================================
BUG: KMSAN: uninit-value in pick_link+0xd8c/0x1690 fs/namei.c:1850
 pick_link+0xd8c/0x1690 fs/namei.c:1850
 step_into+0x156f/0x1640 fs/namei.c:1909
 open_last_lookups fs/namei.c:3674 [inline]
 path_openat+0x39da/0x6100 fs/namei.c:3883
 do_filp_open+0x20e/0x590 fs/namei.c:3913
 do_sys_openat2+0x1bf/0x2f0 fs/open.c:1416
 do_sys_open fs/open.c:1431 [inline]
 __do_sys_openat fs/open.c:1447 [inline]
 __se_sys_openat fs/open.c:1442 [inline]
 __x64_sys_openat+0x2a1/0x310 fs/open.c:1442
 x64_sys_call+0x1fe/0x3c10 arch/x86/include/generated/asm/syscalls_64.h:258
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
 __alloc_pages_noprof+0x9d6/0xe70 mm/page_alloc.c:4719
 alloc_pages_mpol_noprof+0x299/0x990 mm/mempolicy.c:2263
 alloc_pages_noprof mm/mempolicy.c:2343 [inline]
 folio_alloc_noprof+0x1db/0x310 mm/mempolicy.c:2350
 filemap_alloc_folio_noprof+0xa6/0x440 mm/filemap.c:1008
 do_read_cache_folio+0x12a/0xfd0 mm/filemap.c:3753
 do_read_cache_page mm/filemap.c:3855 [inline]
 read_cache_page+0x63/0x1d0 mm/filemap.c:3864
 read_mapping_page include/linux/pagemap.h:907 [inline]
 page_get_link+0x73/0xab0 fs/namei.c:5272
 pick_link+0xd6c/0x1690
 step_into+0x156f/0x1640 fs/namei.c:1909
 open_last_lookups fs/namei.c:3674 [inline]
 path_openat+0x39da/0x6100 fs/namei.c:3883
 do_filp_open+0x20e/0x590 fs/namei.c:3913
 do_sys_openat2+0x1bf/0x2f0 fs/open.c:1416
 do_sys_open fs/open.c:1431 [inline]
 __do_sys_openat fs/open.c:1447 [inline]
 __se_sys_openat fs/open.c:1442 [inline]
 __x64_sys_openat+0x2a1/0x310 fs/open.c:1442
 x64_sys_call+0x1fe/0x3c10 arch/x86/include/generated/asm/syscalls_64.h:258
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

CPU: 0 UID: 0 PID: 5191 Comm: syz-executor190 Not tainted 6.10.0-syzkaller-12708-g2f8c4f506285 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/27/2024
=====================================================


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

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, this is because the
corresponding folio was not found from the mapping, and the memory was
not initialized when allocating a new folio for the filemap.

To avoid the occurrence of kmsan report uninit-value, initialize the
newly allocated folio memory to 0.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 mm/filemap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 657bcd887fdb..1b22eab691e8 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3753,6 +3753,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
 		folio = filemap_alloc_folio(gfp, 0);
 		if (!folio)
 			return ERR_PTR(-ENOMEM);
+
+		void *kaddr = kmap_local_folio(folio, 0);
+		memset(kaddr, 0, folio_size(folio));
+		kunmap_local(kaddr);
+
 		err = filemap_add_folio(mapping, folio, index, gfp);
 		if (unlikely(err)) {
 			folio_put(folio);
-- 
2.43.0

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Al Viro]

On Thu, Aug 01, 2024 at 10:27:39AM +0800, Lizhi Xu wrote:
> syzbot report KMSAN: uninit-value in pick_link, this is because the
> corresponding folio was not found from the mapping, and the memory was
> not initialized when allocating a new folio for the filemap.
> 
> To avoid the occurrence of kmsan report uninit-value, initialize the
> newly allocated folio memory to 0.

NAK.

You are papering over the real bug here.

That page either
	* has been returned by find_get_page(), cached, uptodate and
with uninitialized contents or
	* has been returned by successful read_mapping_page() - and
left with uninitialized contents or
	* had inode->i_size in excess of initialized contents.

I'd suggest bisecting that.

Re: Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Lizhi Xu]

> > syzbot report KMSAN: uninit-value in pick_link, this is because the
> > corresponding folio was not found from the mapping, and the memory was
> > not initialized when allocating a new folio for the filemap.
> >
> > To avoid the occurrence of kmsan report uninit-value, initialize the
> > newly allocated folio memory to 0.
> 
> NAK.
> 
> You are papering over the real bug here.
Did you see the splat? I think you didn't see that.
> 
> That page either
> 	* has been returned by find_get_page(), cached, uptodate and
> with uninitialized contents or
> 	* has been returned by successful read_mapping_page() - and
> left with uninitialized contents or
> 	* had inode->i_size in excess of initialized contents.
> 
> I'd suggest bisecting that.

Re: Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Al Viro]

On Thu, Aug 01, 2024 at 01:28:37PM +0800, Lizhi Xu wrote:
> > > syzbot report KMSAN: uninit-value in pick_link, this is because the
> > > corresponding folio was not found from the mapping, and the memory was
> > > not initialized when allocating a new folio for the filemap.
> > >
> > > To avoid the occurrence of kmsan report uninit-value, initialize the
> > > newly allocated folio memory to 0.
> > 
> > NAK.
> > 
> > You are papering over the real bug here.
> Did you see the splat? I think you didn't see that.

Sigh...  It is stepping into uninitialized data in pick_link(), and by
the look of traces it's been created by page_get_link().

What page_get_link() does is reading from page cache of symlink;
the contents should have come from ->read_folio() (if it's really
a symlink on squashfs, that would be squashfs_symlink_read_folio()).

Uninit might have happened if
	* ->read_folio() hadn't been called at all (which is an obvios
bug - that's what should've read the symlink contents) or
	* ->read_folio() had been called, it failed and yet we are
still trying to use the resulting page.  Again, an obvious bug - if
trying to read fails, we should _not_ use the results or leave it
in page cache for subsequent callers.
	* ->read_folio() had been called, claimed to have succeeded and
yet it had left something in range 0..inode->i_size-1 uninitialized.
Again, a bug, this time in ->read_folio() instance.

Your patch is basically "fill the page with zeroes before reading anything
into it".  It makes KMSAM warning STFU, but it does not fix anything
in any of those cases.

Re: Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Al Viro]

On Thu, Aug 01, 2024 at 08:10:16AM +0100, Al Viro wrote:

> Your patch is basically "fill the page with zeroes before reading anything
> into it".  It makes KMSAM warning STFU, but it does not fix anything
> in any of those cases.

Incidentally, it does that before it goes ahead and calls filemap_read_folio(),
with ->read_folio() as callback.  So if it does make it STFU, the case 1 (->read_folio()
not called at all) is out of running.  Would be interesting to see if that particular
->read_folio() is returning errors and if so - what errors are actually returned.

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Lizhi Xu]

On Thu, 1 Aug 2024 08:10:16 +0100, Al Viro wrote:
> > > > syzbot report KMSAN: uninit-value in pick_link, this is because the
> > > > corresponding folio was not found from the mapping, and the memory was
> > > > not initialized when allocating a new folio for the filemap.
> > > >
> > > > To avoid the occurrence of kmsan report uninit-value, initialize the
> > > > newly allocated folio memory to 0.
> > >
> > > NAK.
> > >
> > > You are papering over the real bug here.
> > Did you see the splat? I think you didn't see that.
> 
> Sigh...  It is stepping into uninitialized data in pick_link(), and by
> the look of traces it's been created by page_get_link().
> 
> What page_get_link() does is reading from page cache of symlink;
> the contents should have come from ->read_folio() (if it's really
> a symlink on squashfs, that would be squashfs_symlink_read_folio()).
> 
> Uninit might have happened if
> 	* ->read_folio() hadn't been called at all (which is an obvios
> bug - that's what should've read the symlink contents) or
> 	* ->read_folio() had been called, it failed and yet we are
> still trying to use the resulting page.  Again, an obvious bug - if
> trying to read fails, we should _not_ use the results or leave it
> in page cache for subsequent callers.
> 	* ->read_folio() had been called, claimed to have succeeded and
> yet it had left something in range 0..inode->i_size-1 uninitialized.
> Again, a bug, this time in ->read_folio() instance.
read_folio, have you noticed that the file value was passed to read_folio is NULL? 
fs/namei.c
const char *page_get_link(struct dentry *dentry, struct inode *inode
...
5272  read_mapping_page(mapping, 0, NULL);

So, Therefore, no matter what, the value of folio will not be initialized by file
in read_folio. 

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Lizhi Xu]

On Thu, 1 Aug 2024 16:12:24 +0800, Lizhi Xu wrote:
> > > > > syzbot report KMSAN: uninit-value in pick_link, this is because the
> > > > > corresponding folio was not found from the mapping, and the memory was
> > > > > not initialized when allocating a new folio for the filemap.
> > > > >
> > > > > To avoid the occurrence of kmsan report uninit-value, initialize the
> > > > > newly allocated folio memory to 0.
> > > >
> > > > NAK.
> > > >
> > > > You are papering over the real bug here.
> > > Did you see the splat? I think you didn't see that.
> > 
> > Sigh...  It is stepping into uninitialized data in pick_link(), and by
> > the look of traces it's been created by page_get_link().
> > 
> > What page_get_link() does is reading from page cache of symlink;
> > the contents should have come from ->read_folio() (if it's really
> > a symlink on squashfs, that would be squashfs_symlink_read_folio()).
> > 
> > Uninit might have happened if
> > 	* ->read_folio() hadn't been called at all (which is an obvios
> > bug - that's what should've read the symlink contents) or
> > 	* ->read_folio() had been called, it failed and yet we are
> > still trying to use the resulting page.  Again, an obvious bug - if
> > trying to read fails, we should _not_ use the results or leave it
> > in page cache for subsequent callers.
> > 	* ->read_folio() had been called, claimed to have succeeded and
> > yet it had left something in range 0..inode->i_size-1 uninitialized.
> > Again, a bug, this time in ->read_folio() instance.
> read_folio, have you noticed that the file value was passed to read_folio is NULL? 
> fs/namei.c
> const char *page_get_link(struct dentry *dentry, struct inode *inode
> ...
> 5272  read_mapping_page(mapping, 0, NULL);
> 
> So, Therefore, no matter what, the value of folio will not be initialized by file
> in read_folio. 
Oh, in read_folio, it will use mapping->host to init folio, I will research
why not init in do_read_cache_folio.

--
Lizhi

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Al Viro]

On Thu, Aug 01, 2024 at 04:12:24PM +0800, Lizhi Xu wrote:
> > 	* ->read_folio() had been called, claimed to have succeeded and
> > yet it had left something in range 0..inode->i_size-1 uninitialized.
> > Again, a bug, this time in ->read_folio() instance.
> read_folio, have you noticed that the file value was passed to read_folio is NULL? 
> fs/namei.c
> const char *page_get_link(struct dentry *dentry, struct inode *inode
> ...
> 5272  read_mapping_page(mapping, 0, NULL);
> 
> So, Therefore, no matter what, the value of folio will not be initialized by file
> in read_folio. 

What does struct file have to do with anything?  What it asks is the
first page of the address space of inode in question.

file argument of ->read_folio() is not how an instance determines which
filesystem object it's dealing with.  _That_ is determined by the
address space (mapping) the folio had been attached to.  For some
filesystems that is not enough - they need an information established
at open() time.  Those ->read_folio() instances can pick such stuff
from the file argument - and those obviously cannot be used with
page_get_link(), since for symlinks there's no opened files, etc.

Most of the instances do not use the 'file' argument.  In particular,
squashfs_symlink_read_folio() doesn't even look at it.

It would probably be less confusing if the arguments of ->read_folio()
went in the opposite order, but that's a separate story.  In any case,
"which filesystem object" is determined by folio->mapping, "which
offset in that filesystem object" comes from folio_pos(folio), not
that it realistically could be anything other than 0 in case of a symlink
(they can't be more than 4Kb long, so the first page will cover the
entire thing).

[PATCH V2] squashfs: Add length check in squashfs_symlink_read_folio

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The incorrect value of length is due to the incorrect value of inode->i_size.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/symlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/squashfs/symlink.c b/fs/squashfs/symlink.c
index 6ef735bd841a..d5fa5165ddd6 100644
--- a/fs/squashfs/symlink.c
+++ b/fs/squashfs/symlink.c
@@ -61,6 +61,12 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
 		}
 	}
 
+	if (length < 0) {
+		ERROR("Unable to read symlink, wrong length [%d]\n", length);
+		error = -EINVAL;
+		goto out;
+	}
+
 	/*
 	 * Read length bytes from symlink metadata.  Squashfs_read_metadata
 	 * is not used here because it can sleep and we want to use
-- 
2.43.0

Re: [PATCH V2] squashfs: Add length check in squashfs_symlink_read_folio

[Jan Kara]

On Thu 01-08-24 23:17:40, Lizhi Xu wrote:
> syzbot report KMSAN: uninit-value in pick_link, the root cause is that
> squashfs_symlink_read_folio did not check the length, resulting in folio
> not being initialized and did not return the corresponding error code.
> 
> The incorrect value of length is due to the incorrect value of inode->i_size.
> 
> Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/squashfs/symlink.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/squashfs/symlink.c b/fs/squashfs/symlink.c
> index 6ef735bd841a..d5fa5165ddd6 100644
> --- a/fs/squashfs/symlink.c
> +++ b/fs/squashfs/symlink.c
> @@ -61,6 +61,12 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
>  		}
>  	}
>  
> +	if (length < 0) {

OK, so this would mean that (int)inode->i_size is a negative number.
Possible. Perhaps we should rather better validate i_size of symlinks in
squashfs_read_inode()? Otherwise it would be a whack-a-mole game of
catching places that get confused by bogus i_size...

								Honza

> +		ERROR("Unable to read symlink, wrong length [%d]\n", length);
> +		error = -EINVAL;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Read length bytes from symlink metadata.  Squashfs_read_metadata
>  	 * is not used here because it can sleep and we want to use
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH V2] squashfs: Add length check in squashfs_symlink_read_folio

[Lizhi Xu]

On Thu, 1 Aug 2024 17:30:42 +0200, Jan Kara wrote:
> > syzbot report KMSAN: uninit-value in pick_link, the root cause is that
> > squashfs_symlink_read_folio did not check the length, resulting in folio
> > not being initialized and did not return the corresponding error code.
> > 
> > The incorrect value of length is due to the incorrect value of inode->i_size.
> > 
> > Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  fs/squashfs/symlink.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/squashfs/symlink.c b/fs/squashfs/symlink.c
> > index 6ef735bd841a..d5fa5165ddd6 100644
> > --- a/fs/squashfs/symlink.c
> > +++ b/fs/squashfs/symlink.c
> > @@ -61,6 +61,12 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
> >  		}
> >  	}
> >  
> > +	if (length < 0) {
> 
> OK, so this would mean that (int)inode->i_size is a negative number.
> Possible. Perhaps we should rather better validate i_size of symlinks in
> squashfs_read_inode()? Otherwise it would be a whack-a-mole game of
> catching places that get confused by bogus i_size...
This move is tough enough, start from where i_size is initialized.
I will send a v3 patch for it.

--
Lizhi

[PATCH V3] squashfs: Add i_size check in squash_read_inode

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The length is calculated from i_size, so it is necessary to add a check
when i_size is initialized to confirm that its value is correct, otherwise
an error -EINVAL will be returned.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
index 16bd693d0b3a..3c667939f1d7 100644
--- a/fs/squashfs/inode.c
+++ b/fs/squashfs/inode.c
@@ -394,6 +394,11 @@ int squashfs_read_inode(struct inode *inode, long long ino)
 		return -EINVAL;
 	}
 
+	if ((int)inode->i_size < 0) {
+		ERROR("Wrong i_size %d!\n", inode->i_size);
+		return -EINVAL;
+	}
+
 	if (xattr_id != SQUASHFS_INVALID_XATTR && msblk->xattr_id_table) {
 		err = squashfs_xattr_lookup(sb, xattr_id,
 					&squashfs_i(inode)->xattr_count,
-- 
2.43.0

[PATCH V4] squashfs: Add i_size check in squash_read_inode

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The length is calculated from i_size, so it is necessary to add a check
when i_size is initialized to confirm that its value is correct, otherwise
an error -EINVAL will be returned. Strictly, the check only applies to the
symlink type.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
index 16bd693d0b3a..6c5dd225482f 100644
--- a/fs/squashfs/inode.c
+++ b/fs/squashfs/inode.c
@@ -287,6 +287,11 @@ int squashfs_read_inode(struct inode *inode, long long ino)
 		inode->i_mode |= S_IFLNK;
 		squashfs_i(inode)->start = block;
 		squashfs_i(inode)->offset = offset;
+		if ((int)inode->i_size < 0) {
+			ERROR("Wrong i_size %d!\n", inode->i_size);
+			return -EINVAL;
+		}
+
 
 		if (type == SQUASHFS_LSYMLINK_TYPE) {
 			__le32 xattr;
-- 
2.43.0

Re: [PATCH V4] squashfs: Add i_size check in squash_read_inode

[Jan Kara]

On Fri 02-08-24 11:01:14, Lizhi Xu wrote:
> syzbot report KMSAN: uninit-value in pick_link, the root cause is that
> squashfs_symlink_read_folio did not check the length, resulting in folio
> not being initialized and did not return the corresponding error code.
> 
> The length is calculated from i_size, so it is necessary to add a check
> when i_size is initialized to confirm that its value is correct, otherwise
> an error -EINVAL will be returned. Strictly, the check only applies to the
> symlink type.
> 
> Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/squashfs/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
> index 16bd693d0b3a..6c5dd225482f 100644
> --- a/fs/squashfs/inode.c
> +++ b/fs/squashfs/inode.c
> @@ -287,6 +287,11 @@ int squashfs_read_inode(struct inode *inode, long long ino)
>  		inode->i_mode |= S_IFLNK;
>  		squashfs_i(inode)->start = block;
>  		squashfs_i(inode)->offset = offset;
> +		if ((int)inode->i_size < 0) {

Looks good. I think you could actually add even more agressive check like:

		if (inode->i_size > PAGE_SIZE) {

because larger symlink isn't supported by squashfs code anyway.

									Honza

> +			ERROR("Wrong i_size %d!\n", inode->i_size);
> +			return -EINVAL;
> +		}
> +
>  
>  		if (type == SQUASHFS_LSYMLINK_TYPE) {
>  			__le32 xattr;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH V5] squashfs: Add i_size check in squash_read_inode

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The length is calculated from i_size, so it is necessary to add a check
when i_size is initialized to confirm that its value is correct, otherwise
an error -EINVAL will be returned. Strictly, the check only applies to the
symlink type. Add larger symlink check.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
index 16bd693d0b3a..6c5dd225482f 100644
--- a/fs/squashfs/inode.c
+++ b/fs/squashfs/inode.c
@@ -287,6 +287,11 @@ int squashfs_read_inode(struct inode *inode, long long ino)
 		inode->i_mode |= S_IFLNK;
 		squashfs_i(inode)->start = block;
 		squashfs_i(inode)->offset = offset;
+		if ((int)inode->i_size < 0 || inode->i_size > PAGE_SIZE) {
+			ERROR("Wrong i_size %d!\n", inode->i_size);
+			return -EINVAL;
+		}
+
 
 		if (type == SQUASHFS_LSYMLINK_TYPE) {
 			__le32 xattr;
-- 
2.43.0

Re: [PATCH V5] squashfs: Add i_size check in squash_read_inode

[Al Viro]

On Fri, Aug 02, 2024 at 07:16:40PM +0800, Lizhi Xu wrote:
> syzbot report KMSAN: uninit-value in pick_link, the root cause is that
> squashfs_symlink_read_folio did not check the length, resulting in folio
> not being initialized and did not return the corresponding error code.
> 
> The length is calculated from i_size, so it is necessary to add a check
> when i_size is initialized to confirm that its value is correct, otherwise
> an error -EINVAL will be returned. Strictly, the check only applies to the
> symlink type. Add larger symlink check.
> 
> Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/squashfs/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
> index 16bd693d0b3a..6c5dd225482f 100644
> --- a/fs/squashfs/inode.c
> +++ b/fs/squashfs/inode.c
> @@ -287,6 +287,11 @@ int squashfs_read_inode(struct inode *inode, long long ino)
>  		inode->i_mode |= S_IFLNK;
>  		squashfs_i(inode)->start = block;
>  		squashfs_i(inode)->offset = offset;
> +		if ((int)inode->i_size < 0 || inode->i_size > PAGE_SIZE) {
> +			ERROR("Wrong i_size %d!\n", inode->i_size);
> +			return -EINVAL;
> +		}

ITYM something like
		if (le32_to_cpu(sqsh_ino->symlink_size) > PAGE_SIZE) {
			ERROR("Corrupted symlink\n");
			return -EINVAL;
		}

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Lizhi Xu]

On Fri, 2 Aug 2024 14:52:14 +0100, Al Viro wrote:
> > +			ERROR("Wrong i_size %d!\n", inode->i_size);
> > +			return -EINVAL;
> > +		}
> 
> ITYM something like
I do not recommend this type of code, as it would add unnecessary calls
to le32_o_cpu compared to directly using i_size.
> 		if (le32_to_cpu(sqsh_ino->symlink_size) > PAGE_SIZE) {
> 			ERROR("Corrupted symlink\n");
> 			return -EINVAL;
> 		}

--
Lizhi

Re: [PATCH] filemap: Init the newly allocated folio memory to 0 for the filemap

[Al Viro]

On Fri, Aug 02, 2024 at 10:44:15PM +0800, Lizhi Xu wrote:
> On Fri, 2 Aug 2024 14:52:14 +0100, Al Viro wrote:
> > > +			ERROR("Wrong i_size %d!\n", inode->i_size);
> > > +			return -EINVAL;
> > > +		}
> > 
> > ITYM something like
> I do not recommend this type of code, as it would add unnecessary calls
> to le32_o_cpu compared to directly using i_size.
> > 		if (le32_to_cpu(sqsh_ino->symlink_size) > PAGE_SIZE) {
> > 			ERROR("Corrupted symlink\n");
> > 			return -EINVAL;
> > 		}

You do realize that it's inlined, right?  Seriously, compare the
generated code...

[PATCH V6] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The length is calculated from i_size, this case is about symlink, so i_size
value is derived from symlink_size, so it is necessary to add a check
when i_size is initialized to confirm that its value is correct, otherwise
an error -EINVAL will be returned. 

If symlink_size is too large, it will result in a negative value when
calculating length in squashfs_symlink_read_folio, and its value must
be greater than PAGE_SIZE at this time.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/inode.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
index 16bd693d0b3a..bed6764e4461 100644
--- a/fs/squashfs/inode.c
+++ b/fs/squashfs/inode.c
@@ -273,14 +273,21 @@ int squashfs_read_inode(struct inode *inode, long long ino)
 	case SQUASHFS_SYMLINK_TYPE:
 	case SQUASHFS_LSYMLINK_TYPE: {
 		struct squashfs_symlink_inode *sqsh_ino = &squashfs_ino.symlink;
+		loff_t symlink_size;
 
 		err = squashfs_read_metadata(sb, sqsh_ino, &block, &offset,
 				sizeof(*sqsh_ino));
 		if (err < 0)
 			goto failed_read;
 
+		symlink_size = le32_to_cpu(sqsh_ino->symlink_size);
+		if (symlink_size > PAGE_SIZE) {
+			ERROR("Corrupted symlink, size [%llu]\n", symlink_size);
+			return -EINVAL;
+		}
+
 		set_nlink(inode, le32_to_cpu(sqsh_ino->nlink));
-		inode->i_size = le32_to_cpu(sqsh_ino->symlink_size);
+		inode->i_size = symlink_size;
 		inode->i_op = &squashfs_symlink_inode_ops;
 		inode_nohighmem(inode);
 		inode->i_data.a_ops = &squashfs_symlink_aops;
-- 
2.43.0

[PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu]

syzbot report KMSAN: uninit-value in pick_link, the root cause is that
squashfs_symlink_read_folio did not check the length, resulting in folio
not being initialized and did not return the corresponding error code.

The length is calculated from i_size, this case is about symlink, so i_size
value is derived from symlink_size, so it is necessary to add a check to
confirm that symlink_size value is valid, otherwise an error -EINVAL will
be returned. 

If symlink_size is too large, it may result in a negative value when
calculating length in squashfs_symlink_read_folio due to int overflow,
and its value must be greater than PAGE_SIZE at this time.

Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/squashfs/inode.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
index 16bd693d0b3a..bed6764e4461 100644
--- a/fs/squashfs/inode.c
+++ b/fs/squashfs/inode.c
@@ -273,14 +273,21 @@ int squashfs_read_inode(struct inode *inode, long long ino)
 	case SQUASHFS_SYMLINK_TYPE:
 	case SQUASHFS_LSYMLINK_TYPE: {
 		struct squashfs_symlink_inode *sqsh_ino = &squashfs_ino.symlink;
+		loff_t symlink_size;
 
 		err = squashfs_read_metadata(sb, sqsh_ino, &block, &offset,
 				sizeof(*sqsh_ino));
 		if (err < 0)
 			goto failed_read;
 
+		symlink_size = le32_to_cpu(sqsh_ino->symlink_size);
+		if (symlink_size > PAGE_SIZE) {
+			ERROR("Corrupted symlink, size [%llu]\n", symlink_size);
+			return -EINVAL;
+		}
+
 		set_nlink(inode, le32_to_cpu(sqsh_ino->nlink));
-		inode->i_size = le32_to_cpu(sqsh_ino->symlink_size);
+		inode->i_size = symlink_size;
 		inode->i_op = &squashfs_symlink_inode_ops;
 		inode_nohighmem(inode);
 		inode->i_data.a_ops = &squashfs_symlink_aops;
-- 
2.43.0

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Phillip Lougher]

On 03/08/2024 08:43, Lizhi Xu wrote:
> syzbot report KMSAN: uninit-value in pick_link, the root cause is that
> squashfs_symlink_read_folio did not check the length, resulting in folio
> not being initialized and did not return the corresponding error code.
> 
> The length is calculated from i_size, this case is about symlink, so i_size
> value is derived from symlink_size, so it is necessary to add a check to
> confirm that symlink_size value is valid, otherwise an error -EINVAL will
> be returned.
> 
> If symlink_size is too large, it may result in a negative value when
> calculating length in squashfs_symlink_read_folio due to int overflow,
> and its value must be greater than PAGE_SIZE at this time.
> 
> Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>   fs/squashfs/inode.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
> index 16bd693d0b3a..bed6764e4461 100644
> --- a/fs/squashfs/inode.c
> +++ b/fs/squashfs/inode.c
> @@ -273,14 +273,21 @@ int squashfs_read_inode(struct inode *inode, long long ino)
>   	case SQUASHFS_SYMLINK_TYPE:
>   	case SQUASHFS_LSYMLINK_TYPE: {
>   		struct squashfs_symlink_inode *sqsh_ino = &squashfs_ino.symlink;
> +		loff_t symlink_size;
>   
>   		err = squashfs_read_metadata(sb, sqsh_ino, &block, &offset,
>   				sizeof(*sqsh_ino));
>   		if (err < 0)
>   			goto failed_read;
>   
> +		symlink_size = le32_to_cpu(sqsh_ino->symlink_size);
> +		if (symlink_size > PAGE_SIZE) {
> +			ERROR("Corrupted symlink, size [%llu]\n", symlink_size);
> +			return -EINVAL;
> +		}
> +
>   		set_nlink(inode, le32_to_cpu(sqsh_ino->nlink));
> -		inode->i_size = le32_to_cpu(sqsh_ino->symlink_size);
> +		inode->i_size = symlink_size;

NACK. I see no reason to introduce an intermediate variable here.

Please do what Al Viro suggested.

Thanks

Phillip Lougher
--
Squashfs author and maintainer

BTW I have been on vacation since last week, and only saw
this today.

>   		inode->i_op = &squashfs_symlink_inode_ops;
>   		inode_nohighmem(inode);
>   		inode->i_data.a_ops = &squashfs_symlink_aops;

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Al Viro]

On Sun, Aug 04, 2024 at 10:16:05PM +0100, Phillip Lougher wrote:

> NACK. I see no reason to introduce an intermediate variable here.
> 
> Please do what Al Viro suggested.

Alternatively, just check ->i_size after assignment.  loff_t is
always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
Conversion from u32 to s64 is always going to yield a non-negative
result; comparison with PAGE_SIZE is all you need there.

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Phillip Lougher]

On 04/08/2024 22:20, Al Viro wrote:
> On Sun, Aug 04, 2024 at 10:16:05PM +0100, Phillip Lougher wrote:
> 
>> NACK. I see no reason to introduce an intermediate variable here.
>>
>> Please do what Al Viro suggested.
> 
> Alternatively, just check ->i_size after assignment.  loff_t is
> always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
> Conversion from u32 to s64 is always going to yield a non-negative
> result; comparison with PAGE_SIZE is all you need there.

I'm happy with that as well.

Thanks

Phillip

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu]

On Sun, 4 Aug 2024 22:20:34 +0100, Al Viro wrote:
> Alternatively, just check ->i_size after assignment.  loff_t is
> always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
> Conversion from u32 to s64 is always going to yield a non-negative
> result; comparison with PAGE_SIZE is all you need there.
It is int overflow, not others. 
Please see my V7 patch,
Link: https://lore.kernel.org/all/20240803074349.3599957-1-lizhi.xu@windriver.com/

--
Lizhi

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Al Viro]

On Mon, Aug 05, 2024 at 09:02:31AM +0800, Lizhi Xu wrote:
> On Sun, 4 Aug 2024 22:20:34 +0100, Al Viro wrote:
> > Alternatively, just check ->i_size after assignment.  loff_t is
> > always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
> > Conversion from u32 to s64 is always going to yield a non-negative
> > result; comparison with PAGE_SIZE is all you need there.

> It is int overflow, not others. 

Excuse me, what?

> Please see my V7 patch,
> Link: https://lore.kernel.org/all/20240803074349.3599957-1-lizhi.xu@windriver.com/

I have seen your patch.  Integer overflow has nothing to do with
the problem here.

Please, show me an unsigned int value N such that

_Bool mismatch(unsigned int N)
{
	u32 v32 = N;
	loff_t v64 = N;

	return (v32 > PAGE_SIZE) != (v64 > PAGE_SIZE);
}

would yield true if passed that value as an argument.

Note that assignment of le32_to_cpu() result to inode->i_size
does conversion from unsigned 32bit integer type to a signed 64bit
integer type.  Since the range of the former fits into the range of the
latter, conversion preserves value.  In other words, possible values
of inode->i_size after such assignment are from 0 to (loff_t)0xfffffff
and (inode->i_size > PAGE_SIZE) is true in exactly the same cases when
(symlink_size > PAGE_SIZE) is true with your patch.

Again, on all architectures inode->i_size is capable of representing
all values in range 0..4G-1 (for rather obvious reasons - we want the
kernel to be able to work with files larger than 4Gb).  There is
no wraparound of any kind on that assignment.

See https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf,
in particular sections 6.5.16.1[2] and 6.3.1.3[1]

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Christian Brauner]

On Sun, Aug 04, 2024 at 11:31:51PM GMT, Phillip Lougher wrote:
> On 04/08/2024 22:20, Al Viro wrote:
> > On Sun, Aug 04, 2024 at 10:16:05PM +0100, Phillip Lougher wrote:
> > 
> > > NACK. I see no reason to introduce an intermediate variable here.
> > > 
> > > Please do what Al Viro suggested.
> > 
> > Alternatively, just check ->i_size after assignment.  loff_t is
> > always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
> > Conversion from u32 to s64 is always going to yield a non-negative
> > result; comparison with PAGE_SIZE is all you need there.
> 
> I'm happy with that as well.

Fwiw, I think a good way to end this v7+ patch streak is to just tweak
the last version when applying.

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu]

On Mon, 5 Aug 2024 02:40:37 +0100, Al Viro wrote:
> > It is int overflow, not others.
> 
> Excuse me, what?
> 
> > Please see my V7 patch,
> > Link: https://lore.kernel.org/all/20240803074349.3599957-1-lizhi.xu@windriver.com/
> 
> I have seen your patch.  Integer overflow has nothing to do with
> the problem here.
No, the fix to this issue is due to the length overflow of int type caused
by the i_size of loff_t type (in the function squashfs_symlink_read_folio).
> 
> Please, show me an unsigned int value N such that
> 
> _Bool mismatch(unsigned int N)
> {
> 	u32 v32 = N;
> 	loff_t v64 = N;
> 
> 	return (v32 > PAGE_SIZE) != (v64 > PAGE_SIZE);
> }
This always return 0, why are you asking this?
> 
> would yield true if passed that value as an argument.
> 
> Note that assignment of le32_to_cpu() result to inode->i_size
> does conversion from unsigned 32bit integer type to a signed 64bit
> integer type.  Since the range of the former fits into the range of the
> latter, conversion preserves value.  In other words, possible values
> of inode->i_size after such assignment are from 0 to (loff_t)0xfffffff
> and (inode->i_size > PAGE_SIZE) is true in exactly the same cases when
> (symlink_size > PAGE_SIZE) is true with your patch.
> 
> Again, on all architectures inode->i_size is capable of representing
> all values in range 0..4G-1 (for rather obvious reasons - we want the
> kernel to be able to work with files larger than 4Gb).  There is
> no wraparound of any kind on that assignment.
The type of loff_t is long long, so its values range is not 0..4G-1.

--
Lizhi

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Al Viro]

On Tue, Aug 06, 2024 at 10:56:09AM +0800, Lizhi Xu wrote:

> > Please, show me an unsigned int value N such that
> > 
> > _Bool mismatch(unsigned int N)
> > {
> > 	u32 v32 = N;
> > 	loff_t v64 = N;
> > 
> > 	return (v32 > PAGE_SIZE) != (v64 > PAGE_SIZE);
> > }
> This always return 0, why are you asking this?

Because that implies the equivalence between

	symlink_size = le32_to_cpu(something);
	if (symlink_size > PAGE_SIZE)
		return -EINVAL;
	inode->i_size = symlink_size;

and

	inode->i_size = le32_to_cpu(something);
	if (inode->i_size > PAGE_SIZE)
		return -EINVAL;

However, you seem to find some problem in the latter form, and
your explanations of the reasons have been hard to understand.

> > Again, on all architectures inode->i_size is capable of representing
> > all values in range 0..4G-1 (for rather obvious reasons - we want the
> > kernel to be able to work with files larger than 4Gb).  There is
> > no wraparound of any kind on that assignment.

> The type of loff_t is long long, so its values range is not 0..4G-1.

6.3.1.3[1] When a value with integer type is converted to another integer type
other than _Bool, if the value can be represented by the new type, it is unchanged.

Possible values of u32 are all in range 0..4G-1.  All numbers in that range
(and many others as well, but that is irrelevant here) can be represented by
loff_t.  In other words, nothing overflow-related is happening.  

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu]

On Tue, 6 Aug 2024 05:59:05 +0100, Al Viro wrote:
> > > Please, show me an unsigned int value N such that
> > >
> > > _Bool mismatch(unsigned int N)
> > > {
> > > 	u32 v32 = N;
> > > 	loff_t v64 = N;
> > >
> > > 	return (v32 > PAGE_SIZE) != (v64 > PAGE_SIZE);
> > > }
> > This always return 0, why are you asking this?
> 
> Because that implies the equivalence between
> 
> 	symlink_size = le32_to_cpu(something);
> 	if (symlink_size > PAGE_SIZE)
> 		return -EINVAL;
> 	inode->i_size = symlink_size;
> 
> and
> 
> 	inode->i_size = le32_to_cpu(something);
> 	if (inode->i_size > PAGE_SIZE)
> 		return -EINVAL;
> 
> However, you seem to find some problem in the latter form, and
> your explanations of the reasons have been hard to understand.
Here are the uninit-value related calltrace reports from syzbot:

page_get_link()->
  read_mapping_page()->
    read_cache_page()->
      do_read_cache_page()->
        do_read_cache_folio()->
          filemap_read_folio()->
            squashfs_symlink_read_folio()

fs/squashfs/symlink.c
 8 static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
 7 {
 6         struct inode *inode = folio->mapping->host;
 5         struct super_block *sb = inode->i_sb;
 4         struct squashfs_sb_info *msblk = sb->s_fs_info;
 3         int index = folio_pos(folio);
 2         u64 block = squashfs_i(inode)->start;
 1         int offset = squashfs_i(inode)->offset;
41         int length = min_t(int, i_size_read(inode) - index, PAGE_SIZE);

Please see line 41, because the value of i_size is too large, causing integer overflow
in the variable length. Which can result in folio not being initialized (as reported by 
Syzbot: "KMSAN: uninit-value in pick_link").

My solution is to check if the value of symlink_size is too large before
initializing i_size with symlink_size. If it is, return -EINVAL.
> 
> > > Again, on all architectures inode->i_size is capable of representing
> > > all values in range 0..4G-1 (for rather obvious reasons - we want the
> > > kernel to be able to work with files larger than 4Gb).  There is
> > > no wraparound of any kind on that assignment.
> 
> > The type of loff_t is long long, so its values range is not 0..4G-1.
> 
> 6.3.1.3[1] When a value with integer type is converted to another integer type
> other than _Bool, if the value can be represented by the new type, it is unchanged.
> 
> Possible values of u32 are all in range 0..4G-1.  All numbers in that range
> (and many others as well, but that is irrelevant here) can be represented by
> loff_t.  In other words, nothing overflow-related is happening.

--
Lizhi

Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Al Viro]

On Tue, Aug 06, 2024 at 02:19:12PM +0800, Lizhi Xu wrote:

> > However, you seem to find some problem in the latter form, and
> > your explanations of the reasons have been hard to understand.
> Here are the uninit-value related calltrace reports from syzbot:
> 
> page_get_link()->
>   read_mapping_page()->
>     read_cache_page()->
>       do_read_cache_page()->
>         do_read_cache_folio()->
>           filemap_read_folio()->
>             squashfs_symlink_read_folio()
> 
> fs/squashfs/symlink.c
>  8 static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
>  7 {
>  6         struct inode *inode = folio->mapping->host;
>  5         struct super_block *sb = inode->i_sb;
>  4         struct squashfs_sb_info *msblk = sb->s_fs_info;
>  3         int index = folio_pos(folio);
>  2         u64 block = squashfs_i(inode)->start;
>  1         int offset = squashfs_i(inode)->offset;
> 41         int length = min_t(int, i_size_read(inode) - index, PAGE_SIZE);
> 
> Please see line 41, because the value of i_size is too large, causing integer overflow
> in the variable length. Which can result in folio not being initialized (as reported by 
> Syzbot: "KMSAN: uninit-value in pick_link").

What does that have to do with anything?  In the code you've quoted, ->i_size - index
is explicitly cast to signed 32bit.  _That_ will wrap around.  On typecast.  Nothing
of that sort would be present in
	if (inode->i_size > PAGE_SIZE)
as you could have verified easily.

At that point the only thing I can recommend is googling for "first law of holes".