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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: bae80473f7b0 ("ext4: use inode_set_cached_link()")
> Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> ---
>
> Per my comments in:
> https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@xxxxxxxxxxxxxx/#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@xxxxxxxxxx/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@xxxxxxxxxxxxxx/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
>
>
