fs/squashfs/inode.c | 5 +++++ 1 file changed, 5 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 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 - 2025 Red Hat, Inc.