Hello Sascha, On 9/4/26 1:13 PM, Sascha Hauer wrote: > On 2026-09-04 13:05, Ahmad Fatoum wrote: >> >> >> On 9/4/26 11:15 AM, Sascha Hauer wrote: >>> f_size is an alias for the inode's i_size, and filesystems read that >>> size straight from untrusted media. ext4's ext4_isize() builds it as >>> ((loff_t)size_high << 32) | size, so bit 31 of size_high lands in the >>> sign bit; squashfs takes an unchecked le64 for LREG inodes. A negative >>> i_size then feeds the offset arithmetic in __read(), __write() and >>> friends, where it either wraps a size_t count to something huge or -- >>> depending on whether size_t is 32 or 64 bit -- turns the comparison >>> unsigned and skips the EOF clamp altogether. >>> >>> Catch this the way Linux does: give the superblock a ceiling and refuse >>> sizes beyond it, instead of hardening every arithmetic site. Default >>> s_maxbytes to MAX_LFS_FILESIZE in init_super(), which runs before the >>> driver probe, so the filesystems that already lower it (jffs2, ubifs, >>> squashfs, 9p) keep doing so and everyone else stops sitting at zero. >>> >>> The check goes into do_dentry_open() rather than into iget: ten drivers >>> only learn the size in their ->open() callback and write it to the >>> inode through file->f_size, so a lookup time check would miss them. >>> Casting to u64 makes a negative size exceed any sane s_maxbytes, so the >>> sign is covered too; FILE_SIZE_STREAM is negative on purpose and has to >>> be excluded. >>> >>> Assisted-by: Claude:claude-opus-5 >>> Signed-off-by: Sascha Hauer <[email protected]> >>> --- >>> fs/fs.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/fs/fs.c b/fs/fs.c >>> index ce41f23f88..9518f28539 100644 >>> --- a/fs/fs.c >>> +++ b/fs/fs.c >>> @@ -986,6 +986,7 @@ int fsdev_open_cdev(struct fs_device *fsdev) >>> static void init_super(struct super_block *sb) >>> { >>> INIT_LIST_HEAD(&sb->s_inodes); >>> + sb->s_maxbytes = MAX_LFS_FILESIZE; >>> } >>> >>> static int fsdev_umount(struct fs_device *fsdev) >>> @@ -2613,6 +2614,14 @@ static int rmdirat(int dirfd, const char *pathname) >>> return errno_set(error); >>> } >>> >>> +static bool i_size_valid(struct inode *inode) >>> +{ >>> + if (inode->i_size == FILE_SIZE_STREAM) >>> + return true; >> >> Is FILE_SIZE_STREAM not a barebox convention? Couldn't a file system >> have on disk FILE_SIZE_STREAM and trigger misbehavior this way? > > Hm, right. At this point we cannot distinguish between a valid > FILE_SIZE_STREAM and a value from a corrupted filesystem. Maybe we > should make this an extra field in struct inode rather than overloading > i_size with this information.
Sounds good. > > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
