mm/filemap.c | 5 +++++ 1 file changed, 5 insertions(+)
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.0On 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.
> > 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.
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.
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.
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).
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.0On 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
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
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.0syzbot 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.0On 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
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.0On 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;
}
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
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...
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.0syzbot 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.0On 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;
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.
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
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]
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
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.
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
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".
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
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.
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
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.
© 2016 - 2026 Red Hat, Inc.