* Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() [not found] <20250205162819.380864-1-mjguzik@gmail.com> @ 2025-02-05 17:29 ` Darrick J. Wong 2025-02-05 17:33 ` Mateusz Guzik 0 siblings, 1 reply; 5+ messages in thread From: Darrick J. Wong @ 2025-02-05 17:29 UTC (permalink / raw) To: Mateusz Guzik Cc: brauner, tytso, kees, viro, jack, linux-kernel, linux-fsdevel, syzbot+48a99e426f29859818c0, linux-ext4 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() 2025-02-05 17:29 ` [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() Darrick J. Wong @ 2025-02-05 17:33 ` Mateusz Guzik 2025-02-05 18:10 ` Jan Kara 0 siblings, 1 reply; 5+ messages in thread From: Mateusz Guzik @ 2025-02-05 17:33 UTC (permalink / raw) To: Darrick J. Wong Cc: brauner, tytso, kees, viro, jack, linux-kernel, linux-fsdevel, syzbot+48a99e426f29859818c0, linux-ext4 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() 2025-02-05 17:33 ` Mateusz Guzik @ 2025-02-05 18:10 ` Jan Kara 2025-02-05 19:05 ` Mateusz Guzik 0 siblings, 1 reply; 5+ messages in thread From: Jan Kara @ 2025-02-05 18:10 UTC (permalink / raw) To: Mateusz Guzik Cc: Darrick J. Wong, brauner, tytso, kees, viro, jack, linux-kernel, linux-fsdevel, syzbot+48a99e426f29859818c0, linux-ext4 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() 2025-02-05 18:10 ` Jan Kara @ 2025-02-05 19:05 ` Mateusz Guzik 2025-02-06 9:44 ` Christian Brauner 0 siblings, 1 reply; 5+ messages in thread From: Mateusz Guzik @ 2025-02-05 19:05 UTC (permalink / raw) To: Jan Kara Cc: Darrick J. Wong, brauner, tytso, kees, viro, linux-kernel, linux-fsdevel, syzbot+48a99e426f29859818c0, linux-ext4 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() 2025-02-05 19:05 ` Mateusz Guzik @ 2025-02-06 9:44 ` Christian Brauner 0 siblings, 0 replies; 5+ messages in thread From: Christian Brauner @ 2025-02-06 9:44 UTC (permalink / raw) To: Mateusz Guzik Cc: Jan Kara, Darrick J. Wong, tytso, kees, viro, linux-kernel, linux-fsdevel, syzbot+48a99e426f29859818c0, linux-ext4 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-06 9:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250205162819.380864-1-mjguzik@gmail.com>
2025-02-05 17:29 ` [PATCH] ext4: pass strlen() of the symlink instead of i_size to inode_set_cached_link() Darrick J. Wong
2025-02-05 17:33 ` Mateusz Guzik
2025-02-05 18:10 ` Jan Kara
2025-02-05 19:05 ` Mateusz Guzik
2025-02-06 9:44 ` Christian Brauner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox