diff mbox series

ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link()

Message ID 20250205162819.380864-1-mjguzik@gmail.com (mailing list archive)
State New
Headers show
Series ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() | expand

Commit Message

Mateusz Guzik Feb. 5, 2025, 4:28 p.m. UTC 
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

 fs/ext4/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mateusz Guzik Feb. 5, 2025, 4:30 p.m. UTC | #1 
On Wed, Feb 5, 2025 at 5:28 PM Mateusz Guzik <mjguzik@gmail.com> 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.
>

oof.. that should be:
> inode_set_cached_link() fails to take it into account
Darrick J. Wong Feb. 5, 2025, 5:29 p.m. UTC | #2 
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
> 
>
Mateusz Guzik Feb. 5, 2025, 5:33 p.m. UTC | #3 
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.
Jan Kara Feb. 5, 2025, 6:10 p.m. UTC | #4 
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
Mateusz Guzik Feb. 5, 2025, 7:05 p.m. UTC | #5 
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.
diff mbox series

Patch

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;
 		}