* [RESEND PATCH 0/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() @ 2025-02-07 3:27 Ye Bin 2025-02-07 3:27 ` [RESEND PATCH 1/2] ext4: introduce ITAIL helper Ye Bin 2025-02-07 3:27 ` [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin 0 siblings, 2 replies; 8+ messages in thread From: Ye Bin @ 2025-02-07 3:27 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: jack From: Ye Bin <yebin10@huawei.com> Ye Bin (2): ext4: introduce ITAIL helper ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() fs/ext4/xattr.c | 24 ++++++++++++++++-------- fs/ext4/xattr.h | 3 +++ 2 files changed, 19 insertions(+), 8 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RESEND PATCH 1/2] ext4: introduce ITAIL helper 2025-02-07 3:27 [RESEND PATCH 0/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin @ 2025-02-07 3:27 ` Ye Bin 2025-02-07 4:15 ` Darrick J. Wong 2025-02-07 3:27 ` [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin 1 sibling, 1 reply; 8+ messages in thread From: Ye Bin @ 2025-02-07 3:27 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: jack From: Ye Bin <yebin10@huawei.com> Introduce ITAIL helper to get the bound of xattr in inode. Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/ext4/xattr.c | 10 +++++----- fs/ext4/xattr.h | 3 +++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 7647e9f6e190..0e4494863d15 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -649,7 +649,7 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name, return error; raw_inode = ext4_raw_inode(&iloc); header = IHDR(inode, raw_inode); - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; + end = ITAIL(inode, raw_inode); error = xattr_check_inode(inode, header, end); if (error) goto cleanup; @@ -793,7 +793,7 @@ ext4_xattr_ibody_list(struct dentry *dentry, char *buffer, size_t buffer_size) return error; raw_inode = ext4_raw_inode(&iloc); header = IHDR(inode, raw_inode); - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; + end = ITAIL(inode, raw_inode); error = xattr_check_inode(inode, header, end); if (error) goto cleanup; @@ -879,7 +879,7 @@ int ext4_get_inode_usage(struct inode *inode, qsize_t *usage) goto out; raw_inode = ext4_raw_inode(&iloc); header = IHDR(inode, raw_inode); - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; + end = ITAIL(inode, raw_inode); ret = xattr_check_inode(inode, header, end); if (ret) goto out; @@ -2235,7 +2235,7 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i, header = IHDR(inode, raw_inode); is->s.base = is->s.first = IFIRST(header); is->s.here = is->s.first; - is->s.end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; + is->s.end = ITAIL(inode, raw_inode); if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { error = xattr_check_inode(inode, header, is->s.end); if (error) @@ -2786,7 +2786,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize, */ base = IFIRST(header); - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; + end = ITAIL(inode, raw_inode); min_offs = end - base; total_ino = sizeof(struct ext4_xattr_ibody_header) + sizeof(u32); diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h index b25c2d7b5f99..d331bd636480 100644 --- a/fs/ext4/xattr.h +++ b/fs/ext4/xattr.h @@ -67,6 +67,9 @@ struct ext4_xattr_entry { ((void *)raw_inode + \ EXT4_GOOD_OLD_INODE_SIZE + \ EXT4_I(inode)->i_extra_isize)) +#define ITAIL(inode, raw_inode) \ + ((void *)raw_inode + \ + EXT4_SB(inode->i_sb)->s_inode_size) #define IFIRST(hdr) ((struct ext4_xattr_entry *)((hdr)+1)) /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 1/2] ext4: introduce ITAIL helper 2025-02-07 3:27 ` [RESEND PATCH 1/2] ext4: introduce ITAIL helper Ye Bin @ 2025-02-07 4:15 ` Darrick J. Wong 0 siblings, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2025-02-07 4:15 UTC (permalink / raw) To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack On Fri, Feb 07, 2025 at 11:27:42AM +0800, Ye Bin wrote: > From: Ye Bin <yebin10@huawei.com> > > Introduce ITAIL helper to get the bound of xattr in inode. > > Signed-off-by: Ye Bin <yebin10@huawei.com> > --- > fs/ext4/xattr.c | 10 +++++----- > fs/ext4/xattr.h | 3 +++ > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 7647e9f6e190..0e4494863d15 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -649,7 +649,7 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name, > return error; > raw_inode = ext4_raw_inode(&iloc); > header = IHDR(inode, raw_inode); > - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; > + end = ITAIL(inode, raw_inode); > error = xattr_check_inode(inode, header, end); > if (error) > goto cleanup; > @@ -793,7 +793,7 @@ ext4_xattr_ibody_list(struct dentry *dentry, char *buffer, size_t buffer_size) > return error; > raw_inode = ext4_raw_inode(&iloc); > header = IHDR(inode, raw_inode); > - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; > + end = ITAIL(inode, raw_inode); > error = xattr_check_inode(inode, header, end); > if (error) > goto cleanup; > @@ -879,7 +879,7 @@ int ext4_get_inode_usage(struct inode *inode, qsize_t *usage) > goto out; > raw_inode = ext4_raw_inode(&iloc); > header = IHDR(inode, raw_inode); > - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; > + end = ITAIL(inode, raw_inode); > ret = xattr_check_inode(inode, header, end); > if (ret) > goto out; > @@ -2235,7 +2235,7 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i, > header = IHDR(inode, raw_inode); > is->s.base = is->s.first = IFIRST(header); > is->s.here = is->s.first; > - is->s.end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; > + is->s.end = ITAIL(inode, raw_inode); > if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { > error = xattr_check_inode(inode, header, is->s.end); > if (error) > @@ -2786,7 +2786,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize, > */ > > base = IFIRST(header); > - end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size; > + end = ITAIL(inode, raw_inode); > min_offs = end - base; > total_ino = sizeof(struct ext4_xattr_ibody_header) + sizeof(u32); > > diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h > index b25c2d7b5f99..d331bd636480 100644 > --- a/fs/ext4/xattr.h > +++ b/fs/ext4/xattr.h > @@ -67,6 +67,9 @@ struct ext4_xattr_entry { > ((void *)raw_inode + \ > EXT4_GOOD_OLD_INODE_SIZE + \ > EXT4_I(inode)->i_extra_isize)) > +#define ITAIL(inode, raw_inode) \ > + ((void *)raw_inode + \ > + EXT4_SB(inode->i_sb)->s_inode_size) These are macros, you ought to wrap the arguments in parentheses to avoid subtle bugs: #define ITAIL(inode, raw_inode) \ ((void *)(raw_inode) + \ EXT4_SB((inode)->i_sb)->s_inode_size) (or maybe just a static inline helper, but I guess we're passing around void pointers so meh) --D > #define IFIRST(hdr) ((struct ext4_xattr_entry *)((hdr)+1)) > > /* > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() 2025-02-07 3:27 [RESEND PATCH 0/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin 2025-02-07 3:27 ` [RESEND PATCH 1/2] ext4: introduce ITAIL helper Ye Bin @ 2025-02-07 3:27 ` Ye Bin 2025-02-07 4:16 ` Darrick J. Wong 1 sibling, 1 reply; 8+ messages in thread From: Ye Bin @ 2025-02-07 3:27 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: jack From: Ye Bin <yebin10@huawei.com> There's issue as follows: BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790 Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172 CPU: 3 PID: 15172 Comm: syz-executor.0 Call Trace: __dump_stack lib/dump_stack.c:82 [inline] dump_stack+0xbe/0xfd lib/dump_stack.c:123 print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400 __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560 kasan_report+0x3a/0x50 mm/kasan/report.c:585 ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137 ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896 ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323 evict+0x39f/0x880 fs/inode.c:622 iput_final fs/inode.c:1746 [inline] iput fs/inode.c:1772 [inline] iput+0x525/0x6c0 fs/inode.c:1758 ext4_orphan_cleanup fs/ext4/super.c:3298 [inline] ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300 mount_bdev+0x355/0x410 fs/super.c:1446 legacy_get_tree+0xfe/0x220 fs/fs_context.c:611 vfs_get_tree+0x8d/0x2f0 fs/super.c:1576 do_new_mount fs/namespace.c:2983 [inline] path_mount+0x119a/0x1ad0 fs/namespace.c:3316 do_mount+0xfc/0x110 fs/namespace.c:3329 __do_sys_mount fs/namespace.c:3540 [inline] __se_sys_mount+0x219/0x2e0 fs/namespace.c:3514 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x67/0xd1 Memory state around the buggy address: ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ^ ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff Above issue happens as ext4_xattr_delete_inode() isn't check xattr is valid if xattr is in inode. To solve above issue call xattr_check_inode() check if xattr if valid in inode. Fixes: e50e5129f384 ("ext4: xattr-in-inode support") Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/ext4/xattr.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 0e4494863d15..cb724477f8da 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, int extra_credits) { struct buffer_head *bh = NULL; - struct ext4_xattr_ibody_header *header; struct ext4_iloc iloc = { .bh = NULL }; struct ext4_xattr_entry *entry; struct inode *ea_inode; @@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, if (ext4_has_feature_ea_inode(inode->i_sb) && ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { + struct ext4_xattr_ibody_header *header; + struct ext4_inode *raw_inode; + void *end; error = ext4_get_inode_loc(inode, &iloc); if (error) { @@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, goto cleanup; } - header = IHDR(inode, ext4_raw_inode(&iloc)); - if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) + raw_inode = ext4_raw_inode(&iloc); + header = IHDR(inode, raw_inode); + end = ITAIL(inode, raw_inode); + if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { + error = xattr_check_inode(inode, header, end); + if (error) + goto cleanup; ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh, IFIRST(header), false /* block_csum */, ea_inode_array, extra_credits, false /* skip_quota */); + } } if (EXT4_I(inode)->i_file_acl) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() 2025-02-07 3:27 ` [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin @ 2025-02-07 4:16 ` Darrick J. Wong 2025-02-07 9:31 ` yebin 0 siblings, 1 reply; 8+ messages in thread From: Darrick J. Wong @ 2025-02-07 4:16 UTC (permalink / raw) To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack On Fri, Feb 07, 2025 at 11:27:43AM +0800, Ye Bin wrote: > From: Ye Bin <yebin10@huawei.com> > > There's issue as follows: > BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790 > Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172 > > CPU: 3 PID: 15172 Comm: syz-executor.0 > Call Trace: > __dump_stack lib/dump_stack.c:82 [inline] > dump_stack+0xbe/0xfd lib/dump_stack.c:123 > print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400 > __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560 > kasan_report+0x3a/0x50 mm/kasan/report.c:585 > ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137 > ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896 > ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323 > evict+0x39f/0x880 fs/inode.c:622 > iput_final fs/inode.c:1746 [inline] > iput fs/inode.c:1772 [inline] > iput+0x525/0x6c0 fs/inode.c:1758 > ext4_orphan_cleanup fs/ext4/super.c:3298 [inline] > ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300 > mount_bdev+0x355/0x410 fs/super.c:1446 > legacy_get_tree+0xfe/0x220 fs/fs_context.c:611 > vfs_get_tree+0x8d/0x2f0 fs/super.c:1576 > do_new_mount fs/namespace.c:2983 [inline] > path_mount+0x119a/0x1ad0 fs/namespace.c:3316 > do_mount+0xfc/0x110 fs/namespace.c:3329 > __do_sys_mount fs/namespace.c:3540 [inline] > __se_sys_mount+0x219/0x2e0 fs/namespace.c:3514 > do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 > entry_SYSCALL_64_after_hwframe+0x67/0xd1 > > Memory state around the buggy address: > ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > ^ > ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > Above issue happens as ext4_xattr_delete_inode() isn't check xattr > is valid if xattr is in inode. > To solve above issue call xattr_check_inode() check if xattr if valid > in inode. > > Fixes: e50e5129f384 ("ext4: xattr-in-inode support") > Signed-off-by: Ye Bin <yebin10@huawei.com> > --- > fs/ext4/xattr.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 0e4494863d15..cb724477f8da 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > int extra_credits) > { > struct buffer_head *bh = NULL; > - struct ext4_xattr_ibody_header *header; > struct ext4_iloc iloc = { .bh = NULL }; > struct ext4_xattr_entry *entry; > struct inode *ea_inode; > @@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > > if (ext4_has_feature_ea_inode(inode->i_sb) && > ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { > + struct ext4_xattr_ibody_header *header; > + struct ext4_inode *raw_inode; > + void *end; > > error = ext4_get_inode_loc(inode, &iloc); > if (error) { > @@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > goto cleanup; > } > > - header = IHDR(inode, ext4_raw_inode(&iloc)); > - if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) > + raw_inode = ext4_raw_inode(&iloc); > + header = IHDR(inode, raw_inode); > + end = ITAIL(inode, raw_inode); > + if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { This needs to make sure that header + sizeof(h_magic) >= end before checking the magic number in header::h_magic, right? --D > + error = xattr_check_inode(inode, header, end); > + if (error) > + goto cleanup; > ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh, > IFIRST(header), > false /* block_csum */, > ea_inode_array, > extra_credits, > false /* skip_quota */); > + } > } > > if (EXT4_I(inode)->i_file_acl) { > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() 2025-02-07 4:16 ` Darrick J. Wong @ 2025-02-07 9:31 ` yebin 2025-02-07 12:34 ` Jan Kara 0 siblings, 1 reply; 8+ messages in thread From: yebin @ 2025-02-07 9:31 UTC (permalink / raw) To: Darrick J. Wong; +Cc: tytso, adilger.kernel, linux-ext4, jack On 2025/2/7 12:16, Darrick J. Wong wrote: > On Fri, Feb 07, 2025 at 11:27:43AM +0800, Ye Bin wrote: >> From: Ye Bin <yebin10@huawei.com> >> >> There's issue as follows: >> BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790 >> Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172 >> >> CPU: 3 PID: 15172 Comm: syz-executor.0 >> Call Trace: >> __dump_stack lib/dump_stack.c:82 [inline] >> dump_stack+0xbe/0xfd lib/dump_stack.c:123 >> print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400 >> __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560 >> kasan_report+0x3a/0x50 mm/kasan/report.c:585 >> ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137 >> ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896 >> ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323 >> evict+0x39f/0x880 fs/inode.c:622 >> iput_final fs/inode.c:1746 [inline] >> iput fs/inode.c:1772 [inline] >> iput+0x525/0x6c0 fs/inode.c:1758 >> ext4_orphan_cleanup fs/ext4/super.c:3298 [inline] >> ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300 >> mount_bdev+0x355/0x410 fs/super.c:1446 >> legacy_get_tree+0xfe/0x220 fs/fs_context.c:611 >> vfs_get_tree+0x8d/0x2f0 fs/super.c:1576 >> do_new_mount fs/namespace.c:2983 [inline] >> path_mount+0x119a/0x1ad0 fs/namespace.c:3316 >> do_mount+0xfc/0x110 fs/namespace.c:3329 >> __do_sys_mount fs/namespace.c:3540 [inline] >> __se_sys_mount+0x219/0x2e0 fs/namespace.c:3514 >> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 >> entry_SYSCALL_64_after_hwframe+0x67/0xd1 >> >> Memory state around the buggy address: >> ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >> ^ >> ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >> ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >> >> Above issue happens as ext4_xattr_delete_inode() isn't check xattr >> is valid if xattr is in inode. >> To solve above issue call xattr_check_inode() check if xattr if valid >> in inode. >> >> Fixes: e50e5129f384 ("ext4: xattr-in-inode support") >> Signed-off-by: Ye Bin <yebin10@huawei.com> >> --- >> fs/ext4/xattr.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c >> index 0e4494863d15..cb724477f8da 100644 >> --- a/fs/ext4/xattr.c >> +++ b/fs/ext4/xattr.c >> @@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >> int extra_credits) >> { >> struct buffer_head *bh = NULL; >> - struct ext4_xattr_ibody_header *header; >> struct ext4_iloc iloc = { .bh = NULL }; >> struct ext4_xattr_entry *entry; >> struct inode *ea_inode; >> @@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >> >> if (ext4_has_feature_ea_inode(inode->i_sb) && >> ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { >> + struct ext4_xattr_ibody_header *header; >> + struct ext4_inode *raw_inode; >> + void *end; >> >> error = ext4_get_inode_loc(inode, &iloc); >> if (error) { >> @@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >> goto cleanup; >> } >> >> - header = IHDR(inode, ext4_raw_inode(&iloc)); >> - if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) >> + raw_inode = ext4_raw_inode(&iloc); >> + header = IHDR(inode, raw_inode); >> + end = ITAIL(inode, raw_inode); >> + if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { > > This needs to make sure that header + sizeof(h_magic) >= end before > checking the magic number in header::h_magic, right? > > --D Thank you for your reply. There ' s no need to check "header + sizeof(h_magic) >= end" because it has been checked when the EXT4_STATE_XATTR flag bit is set: __ext4_iget ret = ext4_iget_extra_inode(inode, raw_inode, ei); if (EXT4_INODE_HAS_XATTR_SPACE(inode) && *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) ext4_set_inode_state(inode, EXT4_STATE_XATTR); It seems that the judgment of "header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)" should be redundant here. > >> + error = xattr_check_inode(inode, header, end); >> + if (error) >> + goto cleanup; >> ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh, >> IFIRST(header), >> false /* block_csum */, >> ea_inode_array, >> extra_credits, >> false /* skip_quota */); >> + } >> } >> >> if (EXT4_I(inode)->i_file_acl) { >> -- >> 2.34.1 >> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() 2025-02-07 9:31 ` yebin @ 2025-02-07 12:34 ` Jan Kara 2025-02-08 1:52 ` yebin 0 siblings, 1 reply; 8+ messages in thread From: Jan Kara @ 2025-02-07 12:34 UTC (permalink / raw) To: yebin; +Cc: Darrick J. Wong, tytso, adilger.kernel, linux-ext4, jack On Fri 07-02-25 17:31:49, yebin wrote: > On 2025/2/7 12:16, Darrick J. Wong wrote: > > On Fri, Feb 07, 2025 at 11:27:43AM +0800, Ye Bin wrote: > > > From: Ye Bin <yebin10@huawei.com> > > > > > > There's issue as follows: > > > BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790 > > > Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172 > > > > > > CPU: 3 PID: 15172 Comm: syz-executor.0 > > > Call Trace: > > > __dump_stack lib/dump_stack.c:82 [inline] > > > dump_stack+0xbe/0xfd lib/dump_stack.c:123 > > > print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400 > > > __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560 > > > kasan_report+0x3a/0x50 mm/kasan/report.c:585 > > > ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137 > > > ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896 > > > ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323 > > > evict+0x39f/0x880 fs/inode.c:622 > > > iput_final fs/inode.c:1746 [inline] > > > iput fs/inode.c:1772 [inline] > > > iput+0x525/0x6c0 fs/inode.c:1758 > > > ext4_orphan_cleanup fs/ext4/super.c:3298 [inline] > > > ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300 > > > mount_bdev+0x355/0x410 fs/super.c:1446 > > > legacy_get_tree+0xfe/0x220 fs/fs_context.c:611 > > > vfs_get_tree+0x8d/0x2f0 fs/super.c:1576 > > > do_new_mount fs/namespace.c:2983 [inline] > > > path_mount+0x119a/0x1ad0 fs/namespace.c:3316 > > > do_mount+0xfc/0x110 fs/namespace.c:3329 > > > __do_sys_mount fs/namespace.c:3540 [inline] > > > __se_sys_mount+0x219/0x2e0 fs/namespace.c:3514 > > > do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 > > > entry_SYSCALL_64_after_hwframe+0x67/0xd1 > > > > > > Memory state around the buggy address: > > > ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > > ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > ^ > > > ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > > > > Above issue happens as ext4_xattr_delete_inode() isn't check xattr > > > is valid if xattr is in inode. > > > To solve above issue call xattr_check_inode() check if xattr if valid > > > in inode. > > > > > > Fixes: e50e5129f384 ("ext4: xattr-in-inode support") > > > Signed-off-by: Ye Bin <yebin10@huawei.com> > > > --- > > > fs/ext4/xattr.c | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > > > index 0e4494863d15..cb724477f8da 100644 > > > --- a/fs/ext4/xattr.c > > > +++ b/fs/ext4/xattr.c > > > @@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > > > int extra_credits) > > > { > > > struct buffer_head *bh = NULL; > > > - struct ext4_xattr_ibody_header *header; > > > struct ext4_iloc iloc = { .bh = NULL }; > > > struct ext4_xattr_entry *entry; > > > struct inode *ea_inode; > > > @@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > > > > > > if (ext4_has_feature_ea_inode(inode->i_sb) && > > > ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { > > > + struct ext4_xattr_ibody_header *header; > > > + struct ext4_inode *raw_inode; > > > + void *end; > > > > > > error = ext4_get_inode_loc(inode, &iloc); > > > if (error) { > > > @@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, > > > goto cleanup; > > > } > > > > > > - header = IHDR(inode, ext4_raw_inode(&iloc)); > > > - if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) > > > + raw_inode = ext4_raw_inode(&iloc); > > > + header = IHDR(inode, raw_inode); > > > + end = ITAIL(inode, raw_inode); > > > + if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { > > > > This needs to make sure that header + sizeof(h_magic) >= end before > > checking the magic number in header::h_magic, right? > > > > --D > Thank you for your reply. > There ' s no need to check "header + sizeof(h_magic) >= end" because it has > been checked > when the EXT4_STATE_XATTR flag bit is set: > __ext4_iget > ret = ext4_iget_extra_inode(inode, raw_inode, ei); > if (EXT4_INODE_HAS_XATTR_SPACE(inode) && *magic == > cpu_to_le32(EXT4_XATTR_MAGIC)) > ext4_set_inode_state(inode, EXT4_STATE_XATTR); > It seems that the judgment of "header->h_magic == > cpu_to_le32(EXT4_XATTR_MAGIC)" > should be redundant here. Yes, if we have EXT4_STATE_XATTR set, xattr_check_inode() should be safe to call (ext4_iget_extra_inode() makes sure inode space is sane) and should return success. I'm actually wondering whether it wouldn't be better for ext4_iget_extra_inode() to check xattr validity with xattr_check_inode() along with setting EXT4_STATE_XATTR. So we'd be checking xattr validity when loading from disk similarly as for xattr blocks which is a well defined place. When doing the checking on use (as we do now) it is always easy to miss some place... Honza > > > + error = xattr_check_inode(inode, header, end); > > > + if (error) > > > + goto cleanup; > > > ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh, > > > IFIRST(header), > > > false /* block_csum */, > > > ea_inode_array, > > > extra_credits, > > > false /* skip_quota */); > > > + } > > > } > > > > > > if (EXT4_I(inode)->i_file_acl) { > > > -- > > > 2.34.1 > > > > > > > > > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() 2025-02-07 12:34 ` Jan Kara @ 2025-02-08 1:52 ` yebin 0 siblings, 0 replies; 8+ messages in thread From: yebin @ 2025-02-08 1:52 UTC (permalink / raw) To: Jan Kara; +Cc: Darrick J. Wong, tytso, adilger.kernel, linux-ext4 On 2025/2/7 20:34, Jan Kara wrote: > On Fri 07-02-25 17:31:49, yebin wrote: >> On 2025/2/7 12:16, Darrick J. Wong wrote: >>> On Fri, Feb 07, 2025 at 11:27:43AM +0800, Ye Bin wrote: >>>> From: Ye Bin <yebin10@huawei.com> >>>> >>>> There's issue as follows: >>>> BUG: KASAN: use-after-free in ext4_xattr_inode_dec_ref_all+0x6ff/0x790 >>>> Read of size 4 at addr ffff88807b003000 by task syz-executor.0/15172 >>>> >>>> CPU: 3 PID: 15172 Comm: syz-executor.0 >>>> Call Trace: >>>> __dump_stack lib/dump_stack.c:82 [inline] >>>> dump_stack+0xbe/0xfd lib/dump_stack.c:123 >>>> print_address_description.constprop.0+0x1e/0x280 mm/kasan/report.c:400 >>>> __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560 >>>> kasan_report+0x3a/0x50 mm/kasan/report.c:585 >>>> ext4_xattr_inode_dec_ref_all+0x6ff/0x790 fs/ext4/xattr.c:1137 >>>> ext4_xattr_delete_inode+0x4c7/0xda0 fs/ext4/xattr.c:2896 >>>> ext4_evict_inode+0xb3b/0x1670 fs/ext4/inode.c:323 >>>> evict+0x39f/0x880 fs/inode.c:622 >>>> iput_final fs/inode.c:1746 [inline] >>>> iput fs/inode.c:1772 [inline] >>>> iput+0x525/0x6c0 fs/inode.c:1758 >>>> ext4_orphan_cleanup fs/ext4/super.c:3298 [inline] >>>> ext4_fill_super+0x8c57/0xba40 fs/ext4/super.c:5300 >>>> mount_bdev+0x355/0x410 fs/super.c:1446 >>>> legacy_get_tree+0xfe/0x220 fs/fs_context.c:611 >>>> vfs_get_tree+0x8d/0x2f0 fs/super.c:1576 >>>> do_new_mount fs/namespace.c:2983 [inline] >>>> path_mount+0x119a/0x1ad0 fs/namespace.c:3316 >>>> do_mount+0xfc/0x110 fs/namespace.c:3329 >>>> __do_sys_mount fs/namespace.c:3540 [inline] >>>> __se_sys_mount+0x219/0x2e0 fs/namespace.c:3514 >>>> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 >>>> entry_SYSCALL_64_after_hwframe+0x67/0xd1 >>>> >>>> Memory state around the buggy address: >>>> ffff88807b002f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>> ffff88807b002f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>>> ffff88807b003000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >>>> ^ >>>> ffff88807b003080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >>>> ffff88807b003100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff >>>> >>>> Above issue happens as ext4_xattr_delete_inode() isn't check xattr >>>> is valid if xattr is in inode. >>>> To solve above issue call xattr_check_inode() check if xattr if valid >>>> in inode. >>>> >>>> Fixes: e50e5129f384 ("ext4: xattr-in-inode support") >>>> Signed-off-by: Ye Bin <yebin10@huawei.com> >>>> --- >>>> fs/ext4/xattr.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c >>>> index 0e4494863d15..cb724477f8da 100644 >>>> --- a/fs/ext4/xattr.c >>>> +++ b/fs/ext4/xattr.c >>>> @@ -2922,7 +2922,6 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >>>> int extra_credits) >>>> { >>>> struct buffer_head *bh = NULL; >>>> - struct ext4_xattr_ibody_header *header; >>>> struct ext4_iloc iloc = { .bh = NULL }; >>>> struct ext4_xattr_entry *entry; >>>> struct inode *ea_inode; >>>> @@ -2937,6 +2936,9 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >>>> >>>> if (ext4_has_feature_ea_inode(inode->i_sb) && >>>> ext4_test_inode_state(inode, EXT4_STATE_XATTR)) { >>>> + struct ext4_xattr_ibody_header *header; >>>> + struct ext4_inode *raw_inode; >>>> + void *end; >>>> >>>> error = ext4_get_inode_loc(inode, &iloc); >>>> if (error) { >>>> @@ -2952,14 +2954,20 @@ int ext4_xattr_delete_inode(handle_t *handle, struct inode *inode, >>>> goto cleanup; >>>> } >>>> >>>> - header = IHDR(inode, ext4_raw_inode(&iloc)); >>>> - if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) >>>> + raw_inode = ext4_raw_inode(&iloc); >>>> + header = IHDR(inode, raw_inode); >>>> + end = ITAIL(inode, raw_inode); >>>> + if (header->h_magic == cpu_to_le32(EXT4_XATTR_MAGIC)) { >>> >>> This needs to make sure that header + sizeof(h_magic) >= end before >>> checking the magic number in header::h_magic, right? >>> >>> --D >> Thank you for your reply. >> There ' s no need to check "header + sizeof(h_magic) >= end" because it has >> been checked >> when the EXT4_STATE_XATTR flag bit is set: >> __ext4_iget >> ret = ext4_iget_extra_inode(inode, raw_inode, ei); >> if (EXT4_INODE_HAS_XATTR_SPACE(inode) && *magic == >> cpu_to_le32(EXT4_XATTR_MAGIC)) >> ext4_set_inode_state(inode, EXT4_STATE_XATTR); >> It seems that the judgment of "header->h_magic == >> cpu_to_le32(EXT4_XATTR_MAGIC)" >> should be redundant here. > > Yes, if we have EXT4_STATE_XATTR set, xattr_check_inode() should be safe to > call (ext4_iget_extra_inode() makes sure inode space is sane) and should > return success. I'm actually wondering whether it wouldn't be better for > ext4_iget_extra_inode() to check xattr validity with xattr_check_inode() > along with setting EXT4_STATE_XATTR. So we'd be checking xattr validity > when loading from disk similarly as for xattr blocks which is a well > defined place. When doing the checking on use (as we do now) it is always > easy to miss some place... > > Honza It's actually better. I'll revise it and post it again. > >>>> + error = xattr_check_inode(inode, header, end); >>>> + if (error) >>>> + goto cleanup; >>>> ext4_xattr_inode_dec_ref_all(handle, inode, iloc.bh, >>>> IFIRST(header), >>>> false /* block_csum */, >>>> ea_inode_array, >>>> extra_credits, >>>> false /* skip_quota */); >>>> + } >>>> } >>>> >>>> if (EXT4_I(inode)->i_file_acl) { >>>> -- >>>> 2.34.1 >>>> >>>> >>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-02-08 1:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-07 3:27 [RESEND PATCH 0/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin 2025-02-07 3:27 ` [RESEND PATCH 1/2] ext4: introduce ITAIL helper Ye Bin 2025-02-07 4:15 ` Darrick J. Wong 2025-02-07 3:27 ` [RESEND PATCH 2/2] ext4: fix out-of-bound read in ext4_xattr_inode_dec_ref_all() Ye Bin 2025-02-07 4:16 ` Darrick J. Wong 2025-02-07 9:31 ` yebin 2025-02-07 12:34 ` Jan Kara 2025-02-08 1:52 ` yebin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox