linux-kernel - Re: [PATCH] ext4: do not BUG when INLINE_DATA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <t6yl3jtspvfby4c6nlqbwjucfkx2evpuebaqvwolgjzcdst3sx@y4yuq7xegul6>
Date: Mon, 21 Jul 2025 14:49:57 +0200
From: Jan Kara <jack@...e.cz>
To: Theodore Ts'o <tytso@....edu>
Cc: Moon Hee Lee <moonhee.lee.ca@...il.com>, 
	syzbot+544248a761451c0df72f@...kaller.appspotmail.com, adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org, 
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com, Jan Kara <jack@...e.cz>, 
	Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org
Subject: Re: [PATCH] ext4: do not BUG when INLINE_DATA_FL lacks system.data
 xattr

On Thu 17-07-25 21:05:21, Theodore Ts'o wrote:
> On Thu, Jul 17, 2025 at 09:59:13AM -0700, Moon Hee Lee wrote:
> > The current patch addresses ext4_update_inline_data() directly, but the
> > same condition also leads to a BUG_ON in ext4_create_inline_data() [2],
> > which the earlier approach intended to prevent as well.
> 
> Actually, the two conditions are opposite to each other.  The one in
> ext4_update_inline_data() was:
> 
>          BUG_ON(is.s.not_found);
> 
> while te one in ext4_create_inline_data() was:
> 
> 	BUG_ON(!is.s.not_found);
> 
> So your patch would not only cause an extra xattr lookup in
> ext4_prepare_inline_data(), but it would actually cause problems by
> causing spurious failures when first writing to an inline data file.
> (Which makes me suspect that you hadn't run other test on your patich
> other than just vaidating that the syzkaller reproduce was no longer
> reproducing.)   
> 
> Also, having taking a closer look at te code paths, I became
> suspicious that there is something about the syzkaller reproducer is
> doing which might be a bit sus.  That's because whether we call
> ext4_update_inline_data() or ext4_create_inline_data() is based on
> whether i_inline off is set or not:
> 
> 	if (ei->i_inline_off)
> 		ret = ext4_update_inline_data(handle, inode, len);
> 	else
> 		ret = ext4_create_inline_data(handle, inode, len);
> 
> 
> But how is ei->i_inline_off set?  It's set from a former call to
> ext4_xattr_ibody_find():
> 
> 	error = ext4_xattr_ibody_find(inode, &i, &is);
> 	if (error)
> 		goto out;
> 
> 	if (!is.s.not_found) {
> 		if (is.s.here->e_value_inum) {
> 			EXT4_ERROR_INODE(inode, "inline data xattr refers "
> 					 "to an external xattr inode");
> 			error = -EFSCORRUPTED;
> 			goto out;
> 		}
> 		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
> 					(void *)ext4_raw_inode(&is.iloc));
> 		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
> 				le32_to_cpu(is.s.here->e_value_size);
> 	}
> 
> So the whole *reason* why i_inline_off exists is because we're caching
> the result of calling ext4_xattr_ibody_find().  So if i_inline_off is
> non-zero, and then when we call ext4_ibody_find() later on, and we
> find that xattr has suddenly disappeared, there is something weird
> going on.   That's why the BUG_ON was added orginally.
> 
> When I took a look at the reproduer, I found that indeed, it is
> calling LOOP_CLR_FD and LOOP_SET_STATUS64 to reconfigure the loop
> device out from under the mounted file system.  This is smashing the
> file system, and is therefore corrupting the block device.  As it
> turns out, Jan Kara recently sent out a patch, and it has been
> accepted in the block tree, to prevent a similar Syzkaller issue using
> LOOP_SET_BLOCK_SIZE[1].
> 
> [1] https://lore.kernel.org/r/20250711163202.19623-2-jack@suse.cz
> 
> We need to do something similar for LOOP_CLR_FD, LOOP_SET_STATUS,
> LOOP_SET_STATUS64, LOOP_CHANGE_FD, and LOOP_SET_CAPACITY ioctls.

Well, careful here. Changing loop device underneath mounted filesystem is a
valid usecase in active use (similarly as changing DM device underneath a
filesystem). So don't think we can play similar tricks as with
LOOP_SET_BLOCK_SIZE where changing block device block size just doesn't
make sense while the device is in use. Similarly LOOP_CLR_FD is an
equivalent of device going away. LOOP_CHANGE_FD is a legacy of the past but
it was *designed* to be used to swap backing file under a life filesystem
(old days of Wild West :)) during boot. We may get away with dropping that
these days but so far I'm not convinced it's worth the risk. So in this case
I don't see anything here that couldn't happen with say DM device and thus
I wouldn't really restrict the loop device functionality...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR