Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

[Darrick J. Wong]

On Wed, Feb 05, 2025 at 05:28:19PM +0100, Mateusz Guzik wrote:
> The call to nd_terminate_link() clamps the size to min(i_size,
> sizeof(ei->i_data) - 1), while the subsequent call to
> inode_set_cached_link() fails the possible update.
> 
> The kernel used to always strlen(), so do it now as well.
> 
> Reported-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com
> Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> Per my comments in:
> https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@mail.gmail.com/#t
> 
> There is definitely a pre-existing bug in ext4 which the above happens
> to run into. I suspect the nd_terminate_link thing will disappear once
> that gets sorted out.
> 
> In the meantime the appropriate fix for 6.14 is to restore the original
> behavior of issuing strlen.
> 
> syzbot verified the issue is fixed:
> https://lore.kernel.org/linux-hardening/67a381a3.050a0220.50516.0077.GAE@google.com/T/#m340e6b52b9547ac85471a1da5980fe0a67c790ac

Again, this is evidence of inconsistent inode metadata, which should be
dealt with by returning EFSCORRUPTED, not arbitrarily truncating the
contents of a bad inode.

https://lore.kernel.org/linux-fsdevel/CAGudoHHeHKo6+R86pZTFSzAFRf2v=bc5LOGvbHmC0mCfkjRvgw@mail.gmail.com/T/#mf05b770926225812f8c78c58c6f3b707c7d151d8

To spell this out -- this is ext4_inode_is_fast_symlink:

int ext4_inode_is_fast_symlink(struct inode *inode)
{
	if (!(EXT4_I(inode)->i_flags & EXT4_EA_INODE_FL)) {
		int ea_blocks = EXT4_I(inode)->i_file_acl ?
				EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;

		if (ext4_has_inline_data(inode))
			return 0;

		return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
	}
	return S_ISLNK(inode->i_mode) && inode->i_size &&
	       (inode->i_size < EXT4_N_BLOCKS * 4);
}

Note in the !EA_INODE and !inlinedata case, the decision is made based
on the block count of the file, without checking i_size.  The callsite
should look more like this:

		} else if (ext4_inode_is_fast_symlink(inode)) {
			if (inode->i_size == 0 ||
			    inode->i_size > sizeof(ei->i_data) - 1) {
				ret = -EFSCORRUPTED;
				ext4_error_inode(..."fast symlink doesn't fit in body??");
				goto bad_inode;
			}
			inode->i_op = &ext4_fast_symlink_inode_operations;
			nd_terminate_link(ei->i_data, inode->i_size,
				sizeof(ei->i_data) - 1);
			inode_set_cached_link(inode, (char *)ei->i_data,
					      inode->i_size);
		} else {

And, seriously, cc the ext4 list on ext4 patches please.

--D

>  fs/ext4/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 7c54ae5fcbd4..30cff983e601 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5010,7 +5010,7 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  			nd_terminate_link(ei->i_data, inode->i_size,
>  				sizeof(ei->i_data) - 1);
>  			inode_set_cached_link(inode, (char *)ei->i_data,
> -					      inode->i_size);
> +					      strlen((char *)ei->i_data));
>  		} else {
>  			inode->i_op = &ext4_symlink_inode_operations;
>  		}
> -- 
> 2.43.0
> 
> 

Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

[Mateusz Guzik]

On Wed, Feb 5, 2025 at 6:29 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Feb 05, 2025 at 05:28:19PM +0100, Mateusz Guzik wrote:
> > The call to nd_terminate_link() clamps the size to min(i_size,
> > sizeof(ei->i_data) - 1), while the subsequent call to
> > inode_set_cached_link() fails the possible update.
> >
> > The kernel used to always strlen(), so do it now as well.
> >
> > Reported-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com
> > Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > ---
> >
> > Per my comments in:
> > https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@mail.gmail.com/#t
> >
> > There is definitely a pre-existing bug in ext4 which the above happens
> > to run into. I suspect the nd_terminate_link thing will disappear once
> > that gets sorted out.
> >
> > In the meantime the appropriate fix for 6.14 is to restore the original
> > behavior of issuing strlen.
> >
> > syzbot verified the issue is fixed:
> > https://lore.kernel.org/linux-hardening/67a381a3.050a0220.50516.0077.GAE@google.com/T/#m340e6b52b9547ac85471a1da5980fe0a67c790ac
>
> Again, this is evidence of inconsistent inode metadata, which should be
> dealt with by returning EFSCORRUPTED, not arbitrarily truncating the
> contents of a bad inode.
>

I agree, rejecting the inode was something I was advocating for from the get go.

I don't know if a real patch(tm) will materialize for 6.14, so in the
meantime I can at least damage-control this back to the original
state.

If the ext4 folk do the right fix, I will be delighted to have this
patch dropped. :)

> And, seriously, cc the ext4 list on ext4 patches please.

Ye that's my bad.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

[Jan Kara]

On Wed 05-02-25 18:33:23, Mateusz Guzik wrote:
> On Wed, Feb 5, 2025 at 6:29 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Wed, Feb 05, 2025 at 05:28:19PM +0100, Mateusz Guzik wrote:
> > > The call to nd_terminate_link() clamps the size to min(i_size,
> > > sizeof(ei->i_data) - 1), while the subsequent call to
> > > inode_set_cached_link() fails the possible update.
> > >
> > > The kernel used to always strlen(), so do it now as well.
> > >
> > > Reported-by: syzbot+48a99e426f29859818c0@syzkaller.appspotmail.com
> > > Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > > ---
> > >
> > > Per my comments in:
> > > https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@mail.gmail.com/#t
> > >
> > > There is definitely a pre-existing bug in ext4 which the above happens
> > > to run into. I suspect the nd_terminate_link thing will disappear once
> > > that gets sorted out.
> > >
> > > In the meantime the appropriate fix for 6.14 is to restore the original
> > > behavior of issuing strlen.
> > >
> > > syzbot verified the issue is fixed:
> > > https://lore.kernel.org/linux-hardening/67a381a3.050a0220.50516.0077.GAE@google.com/T/#m340e6b52b9547ac85471a1da5980fe0a67c790ac
> >
> > Again, this is evidence of inconsistent inode metadata, which should be
> > dealt with by returning EFSCORRUPTED, not arbitrarily truncating the
> > contents of a bad inode.
> >
> 
> I agree, rejecting the inode was something I was advocating for from the get go.
> 
> I don't know if a real patch(tm) will materialize for 6.14, so in the
> meantime I can at least damage-control this back to the original
> state.
> 
> If the ext4 folk do the right fix, I will be delighted to have this
> patch dropped. :)

Yeah, let me cook up proper ext4 fix for this (currently under testing).

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

Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

[Mateusz Guzik]

On Wed, Feb 5, 2025 at 7:10 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 05-02-25 18:33:23, Mateusz Guzik wrote:
> > If the ext4 folk do the right fix, I will be delighted to have this
> > patch dropped. :)
>
> Yeah, let me cook up proper ext4 fix for this (currently under testing).
>

I see it got posted and tested:
https://lore.kernel.org/linux-hardening/67a3b38f.050a0220.19061f.05ea.GAE@google.com/T/#mb782935cc6926dd5642984189d922135f023ec43

So I consider my patch self-NAKed.

Thanks for sorting it out.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

[Christian Brauner]

On Wed, Feb 05, 2025 at 08:05:20PM +0100, Mateusz Guzik wrote:
> On Wed, Feb 5, 2025 at 7:10 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 05-02-25 18:33:23, Mateusz Guzik wrote:
> > > If the ext4 folk do the right fix, I will be delighted to have this
> > > patch dropped. :)
> >
> > Yeah, let me cook up proper ext4 fix for this (currently under testing).
> >
> 
> I see it got posted and tested:
> https://lore.kernel.org/linux-hardening/67a3b38f.050a0220.19061f.05ea.GAE@google.com/T/#mb782935cc6926dd5642984189d922135f023ec43
> 
> So I consider my patch self-NAKed.

Dropped.