fs/squashfs/symlink.c | 6 ++++++ 1 file changed, 6 insertions(+)
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.
© 2016 - 2026 Red Hat, Inc.