| Message ID | 20240803074349.3599957-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V7] squashfs: Add symlink size check in squash_read_inode | expand |
On 03/08/2024 08:43, Lizhi Xu wrote: > syzbot report KMSAN: uninit-value in pick_link, the root cause is that > squashfs_symlink_read_folio did not check the length, resulting in folio > not being initialized and did not return the corresponding error code. > > The length is calculated from i_size, this case is about symlink, so i_size > value is derived from symlink_size, so it is necessary to add a check to > confirm that symlink_size value is valid, otherwise an error -EINVAL will > be returned. > > If symlink_size is too large, it may result in a negative value when > calculating length in squashfs_symlink_read_folio due to int overflow, > and its value must be greater than PAGE_SIZE at this time. > > Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9 > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > --- > fs/squashfs/inode.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c > index 16bd693d0b3a..bed6764e4461 100644 > --- a/fs/squashfs/inode.c > +++ b/fs/squashfs/inode.c > @@ -273,14 +273,21 @@ int squashfs_read_inode(struct inode *inode, long long ino) > case SQUASHFS_SYMLINK_TYPE: > case SQUASHFS_LSYMLINK_TYPE: { > struct squashfs_symlink_inode *sqsh_ino = &squashfs_ino.symlink; > + loff_t symlink_size; > > err = squashfs_read_metadata(sb, sqsh_ino, &block, &offset, > sizeof(*sqsh_ino)); > if (err < 0) > goto failed_read; > > + symlink_size = le32_to_cpu(sqsh_ino->symlink_size); > + if (symlink_size > PAGE_SIZE) { > + ERROR("Corrupted symlink, size [%llu]\n", symlink_size); > + return -EINVAL; > + } > + > set_nlink(inode, le32_to_cpu(sqsh_ino->nlink)); > - inode->i_size = le32_to_cpu(sqsh_ino->symlink_size); > + inode->i_size = symlink_size; NACK. I see no reason to introduce an intermediate variable here. Please do what Al Viro suggested. Thanks Phillip Lougher -- Squashfs author and maintainer BTW I have been on vacation since last week, and only saw this today. > inode->i_op = &squashfs_symlink_inode_ops; > inode_nohighmem(inode); > inode->i_data.a_ops = &squashfs_symlink_aops;
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 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, 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 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 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".
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;
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(-)