Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

[Lizhi Xu <lizhi.xu@windriver.com>]

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