KMSAN: uninit-value in pick_link

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.

Quoted 6 lines

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.

Quoted 10 lines

Quoted 6 lines

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.

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.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

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.

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

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()).

Quoted 10 lines

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.

Quoted 3 lines

* ->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.

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9

Signed-off-by Lizhi Xu <[email protected]>

---
 fs/squashfs/symlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

fs/squashfs/symlink.c +0 -0

index 6ef735bd841a..d5fa5165ddd6 100644

fs/squashfs/symlink.c +5 -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

Signature · 1 line

2.43.0

Quoted 21 lines

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9

Signed-off-by Lizhi Xu <[email protected]>

---
 fs/squashfs/symlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

fs/squashfs/symlink.c +0 -0

index 6ef735bd841a..d5fa5165ddd6 100644
@@ -61,6 +61,12 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
 		}
 	}
 

fs/squashfs/symlink.c static int squashfs_symlink_read_folio(struct file *file, struct folio *folio) · line 64

Quoted 1 line

fs/squashfs/symlink.c +1 -0

+	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...

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9

Signed-off-by Lizhi Xu <[email protected]>

---
 fs/squashfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/squashfs/inode.c +0 -0

index 16bd693d0b3a..6c5dd225482f 100644

fs/squashfs/inode.c +4 -0

+			ERROR("Wrong i_size %d!\n", inode->i_size);
+			return -EINVAL;
+		}
+
 
 		if (type == SQUASHFS_LSYMLINK_TYPE) {
 			__le32 xattr;

Signature · 1 line

2.43.0

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9

Signed-off-by Lizhi Xu <[email protected]>

---
 fs/squashfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/squashfs/inode.c +0 -0

index 16bd693d0b3a..6c5dd225482f 100644

fs/squashfs/inode.c int squashfs_read_inode(struct inode *inode, long long ino) · line 293

Quoted 3 lines

fs/squashfs/inode.c +3 -0

+			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;
		}

Quoted 5 lines · Al Viro

fs/squashfs/inode.c int squashfs_read_inode(struct inode *inode, long long ino) · line 293

Quoted 3 lines

fs/squashfs/inode.c +3 -0

+			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.

Quoted 4 lines

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

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 [email protected]

Closes https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9

Signed-off-by Lizhi Xu <[email protected]>

---
  fs/squashfs/inode.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

fs/squashfs/inode.c +1 -0

index 16bd693d0b3a..bed6764e4461 100644
@@ -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;

fs/squashfs/inode.c +0 -0

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

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

Please do what Al Viro suggested.

Quoted 3 lines · Phillip Lougher

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.

Quoted 4 lines · Al Viro

Quoted 2 lines · Phillip Lougher

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

Quoted 6 lines

Quoted 1 line

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.

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.

Quoted 4 lines · Al Viro

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/[email protected]/ open here

Quoted 1 line

It is int overflow, not others.

Excuse me, what?

Quoted 2 lines

Please see my V7 patch,

Link https://lore.kernel.org/all/[email protected]/ open here

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.

Quoted 9 lines

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?

Quoted 4 lines

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.

Quoted 10 lines

Quoted 9 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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").