[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806061912.3774595-1-lizhi.xu@windriver.com>
Date: Tue, 6 Aug 2024 14:19:12 +0800
From: Lizhi Xu <lizhi.xu@...driver.com>
To: <viro@...iv.linux.org.uk>
CC: <brauner@...nel.org>, <jack@...e.cz>, <linux-fsdevel@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <lizhi.xu@...driver.com>,
<phillip@...ashfs.org.uk>, <squashfs-devel@...ts.sourceforge.net>,
<syzbot+24ac24ff58dc5b0d26b9@...kaller.appspotmail.com>,
<syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode
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
Powered by blists - more mailing lists