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@xxxxxxxxxx> 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@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.
>

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>