* [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
* [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 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
* 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