* [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
@ 2022-06-16 2:13 ` Baokun Li
2022-06-16 4:00 ` Ritesh Harjani
2022-06-16 10:06 ` Jan Kara
2022-06-16 2:13 ` [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
` (4 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-06-16 2:13 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3, libaokun1
When adding an xattr to an inode, we must ensure that the inode_size is
not less than EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad. Otherwise,
the end position may be greater than the start position, resulting in UAF.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/xattr.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 77efb9a627ad..f885f362add4 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -95,6 +95,19 @@ struct ext4_xattr_entry {
#define EXT4_ZERO_XATTR_VALUE ((void *)-1)
+/*
+ * If we want to add an xattr to the inode, we should make sure that
+ * i_extra_isize is not 0 and that the inode size is not less than
+ * EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad.
+ * EXT4_GOOD_OLD_INODE_SIZE extra_isize header entry pad data
+ * |--------------------------|------------|------|---------|---|-------|
+ */
+#define EXT4_INODE_HAS_XATTR_SPACE(inode) \
+ ((EXT4_I(inode)->i_extra_isize != 0) && \
+ (EXT4_GOOD_OLD_INODE_SIZE + EXT4_I(inode)->i_extra_isize + \
+ sizeof(struct ext4_xattr_ibody_header) + EXT4_XATTR_PAD <= \
+ EXT4_INODE_SIZE((inode)->i_sb)))
+
struct ext4_xattr_info {
const char *name;
const void *value;
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
2022-06-16 2:13 ` [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h Baokun Li
@ 2022-06-16 4:00 ` Ritesh Harjani
2022-06-16 10:06 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Ritesh Harjani @ 2022-06-16 4:00 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3
On 22/06/16 10:13AM, Baokun Li wrote:
> When adding an xattr to an inode, we must ensure that the inode_size is
> not less than EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad. Otherwise,
> the end position may be greater than the start position, resulting in UAF.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/xattr.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index 77efb9a627ad..f885f362add4 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -95,6 +95,19 @@ struct ext4_xattr_entry {
>
> #define EXT4_ZERO_XATTR_VALUE ((void *)-1)
>
> +/*
> + * If we want to add an xattr to the inode, we should make sure that
> + * i_extra_isize is not 0 and that the inode size is not less than
> + * EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad.
> + * EXT4_GOOD_OLD_INODE_SIZE extra_isize header entry pad data
> + * |--------------------------|------------|------|---------|---|-------|
> + */
Thanks for adding the visual :)
Looks good to me. Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> +#define EXT4_INODE_HAS_XATTR_SPACE(inode) \
> + ((EXT4_I(inode)->i_extra_isize != 0) && \
> + (EXT4_GOOD_OLD_INODE_SIZE + EXT4_I(inode)->i_extra_isize + \
> + sizeof(struct ext4_xattr_ibody_header) + EXT4_XATTR_PAD <= \
> + EXT4_INODE_SIZE((inode)->i_sb)))
> +
> struct ext4_xattr_info {
> const char *name;
> const void *value;
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
2022-06-16 2:13 ` [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h Baokun Li
2022-06-16 4:00 ` Ritesh Harjani
@ 2022-06-16 10:06 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-06-16 10:06 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list, lczerner,
enwlinux, linux-kernel, yi.zhang, yebin10, yukuai3
On Thu 16-06-22 10:13:55, Baokun Li wrote:
> When adding an xattr to an inode, we must ensure that the inode_size is
> not less than EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad. Otherwise,
> the end position may be greater than the start position, resulting in UAF.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/xattr.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
> index 77efb9a627ad..f885f362add4 100644
> --- a/fs/ext4/xattr.h
> +++ b/fs/ext4/xattr.h
> @@ -95,6 +95,19 @@ struct ext4_xattr_entry {
>
> #define EXT4_ZERO_XATTR_VALUE ((void *)-1)
>
> +/*
> + * If we want to add an xattr to the inode, we should make sure that
> + * i_extra_isize is not 0 and that the inode size is not less than
> + * EXT4_GOOD_OLD_INODE_SIZE + extra_isize + pad.
> + * EXT4_GOOD_OLD_INODE_SIZE extra_isize header entry pad data
> + * |--------------------------|------------|------|---------|---|-------|
> + */
> +#define EXT4_INODE_HAS_XATTR_SPACE(inode) \
> + ((EXT4_I(inode)->i_extra_isize != 0) && \
> + (EXT4_GOOD_OLD_INODE_SIZE + EXT4_I(inode)->i_extra_isize + \
> + sizeof(struct ext4_xattr_ibody_header) + EXT4_XATTR_PAD <= \
> + EXT4_INODE_SIZE((inode)->i_sb)))
> +
> struct ext4_xattr_info {
> const char *name;
> const void *value;
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
2022-06-16 2:13 ` [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h Baokun Li
@ 2022-06-16 2:13 ` Baokun Li
2022-06-16 4:02 ` Ritesh Harjani
2022-06-16 10:07 ` Jan Kara
2022-06-16 2:13 ` [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing Baokun Li
` (3 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-06-16 2:13 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3, libaokun1, Hulk Robot
Hulk Robot reported a issue:
==================================================================
BUG: KASAN: use-after-free in ext4_xattr_set_entry+0x18ab/0x3500
Write of size 4105 at addr ffff8881675ef5f4 by task syz-executor.0/7092
CPU: 1 PID: 7092 Comm: syz-executor.0 Not tainted 4.19.90-dirty #17
Call Trace:
[...]
memcpy+0x34/0x50 mm/kasan/kasan.c:303
ext4_xattr_set_entry+0x18ab/0x3500 fs/ext4/xattr.c:1747
ext4_xattr_ibody_inline_set+0x86/0x2a0 fs/ext4/xattr.c:2205
ext4_xattr_set_handle+0x940/0x1300 fs/ext4/xattr.c:2386
ext4_xattr_set+0x1da/0x300 fs/ext4/xattr.c:2498
__vfs_setxattr+0x112/0x170 fs/xattr.c:149
__vfs_setxattr_noperm+0x11b/0x2a0 fs/xattr.c:180
__vfs_setxattr_locked+0x17b/0x250 fs/xattr.c:238
vfs_setxattr+0xed/0x270 fs/xattr.c:255
setxattr+0x235/0x330 fs/xattr.c:520
path_setxattr+0x176/0x190 fs/xattr.c:539
__do_sys_lsetxattr fs/xattr.c:561 [inline]
__se_sys_lsetxattr fs/xattr.c:557 [inline]
__x64_sys_lsetxattr+0xc2/0x160 fs/xattr.c:557
do_syscall_64+0xdf/0x530 arch/x86/entry/common.c:298
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x459fe9
RSP: 002b:00007fa5e54b4c08 EFLAGS: 00000246 ORIG_RAX: 00000000000000bd
RAX: ffffffffffffffda RBX: 000000000051bf60 RCX: 0000000000459fe9
RDX: 00000000200003c0 RSI: 0000000020000180 RDI: 0000000020000140
RBP: 000000000051bf60 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000001009 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffc73c93fc0 R14: 000000000051bf60 R15: 00007fa5e54b4d80
[...]
==================================================================
Above issue may happen as follows:
-------------------------------------
ext4_xattr_set
ext4_xattr_set_handle
ext4_xattr_ibody_find
>> s->end < s->base
>> no EXT4_STATE_XATTR
>> xattr_check_inode is not executed
ext4_xattr_ibody_set
ext4_xattr_set_entry
>> size_t min_offs = s->end - s->base
>> UAF in memcpy
we can easily reproduce this problem with the following commands:
mkfs.ext4 -F /dev/sda
mount -o debug_want_extra_isize=128 /dev/sda /mnt
touch /mnt/file
setfattr -n user.cat -v `seq -s z 4096|tr -d '[:digit:]'` /mnt/file
In ext4_xattr_ibody_find, we have the following assignment logic:
header = IHDR(inode, raw_inode)
= raw_inode + EXT4_GOOD_OLD_INODE_SIZE + i_extra_isize
is->s.base = IFIRST(header)
= header + sizeof(struct ext4_xattr_ibody_header)
is->s.end = raw_inode + s_inode_size
In ext4_xattr_set_entry
min_offs = s->end - s->base
= s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
sizeof(struct ext4_xattr_ibody_header)
last = s->first
free = min_offs - ((void *)last - s->base) - sizeof(__u32)
= s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
sizeof(struct ext4_xattr_ibody_header) - sizeof(__u32)
In the calculation formula, all values except s_inode_size and
i_extra_size are fixed values. When i_extra_size is the maximum value
s_inode_size - EXT4_GOOD_OLD_INODE_SIZE, min_offs is -4 and free is -8.
The value overflows. As a result, the preceding issue is triggered when
memcpy is executed.
Therefore, when finding xattr or setting xattr, check whether
there is space for storing xattr in the inode to resolve this issue.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/xattr.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 042325349098..c3c3194f3ee1 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2176,8 +2176,9 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
struct ext4_inode *raw_inode;
int error;
- if (EXT4_I(inode)->i_extra_isize == 0)
+ if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
return 0;
+
raw_inode = ext4_raw_inode(&is->iloc);
header = IHDR(inode, raw_inode);
is->s.base = is->s.first = IFIRST(header);
@@ -2205,8 +2206,9 @@ int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
struct ext4_xattr_search *s = &is->s;
int error;
- if (EXT4_I(inode)->i_extra_isize == 0)
+ if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
return -ENOSPC;
+
error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
if (error)
return error;
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry
2022-06-16 2:13 ` [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
@ 2022-06-16 4:02 ` Ritesh Harjani
2022-06-16 10:07 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Ritesh Harjani @ 2022-06-16 4:02 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3, Hulk Robot
On 22/06/16 10:13AM, Baokun Li wrote:
> Hulk Robot reported a issue:
> ==================================================================
> BUG: KASAN: use-after-free in ext4_xattr_set_entry+0x18ab/0x3500
> Write of size 4105 at addr ffff8881675ef5f4 by task syz-executor.0/7092
>
> CPU: 1 PID: 7092 Comm: syz-executor.0 Not tainted 4.19.90-dirty #17
> Call Trace:
> [...]
> memcpy+0x34/0x50 mm/kasan/kasan.c:303
> ext4_xattr_set_entry+0x18ab/0x3500 fs/ext4/xattr.c:1747
> ext4_xattr_ibody_inline_set+0x86/0x2a0 fs/ext4/xattr.c:2205
> ext4_xattr_set_handle+0x940/0x1300 fs/ext4/xattr.c:2386
> ext4_xattr_set+0x1da/0x300 fs/ext4/xattr.c:2498
> __vfs_setxattr+0x112/0x170 fs/xattr.c:149
> __vfs_setxattr_noperm+0x11b/0x2a0 fs/xattr.c:180
> __vfs_setxattr_locked+0x17b/0x250 fs/xattr.c:238
> vfs_setxattr+0xed/0x270 fs/xattr.c:255
> setxattr+0x235/0x330 fs/xattr.c:520
> path_setxattr+0x176/0x190 fs/xattr.c:539
> __do_sys_lsetxattr fs/xattr.c:561 [inline]
> __se_sys_lsetxattr fs/xattr.c:557 [inline]
> __x64_sys_lsetxattr+0xc2/0x160 fs/xattr.c:557
> do_syscall_64+0xdf/0x530 arch/x86/entry/common.c:298
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x459fe9
> RSP: 002b:00007fa5e54b4c08 EFLAGS: 00000246 ORIG_RAX: 00000000000000bd
> RAX: ffffffffffffffda RBX: 000000000051bf60 RCX: 0000000000459fe9
> RDX: 00000000200003c0 RSI: 0000000020000180 RDI: 0000000020000140
> RBP: 000000000051bf60 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000001009 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007ffc73c93fc0 R14: 000000000051bf60 R15: 00007fa5e54b4d80
> [...]
> ==================================================================
>
> Above issue may happen as follows:
> -------------------------------------
> ext4_xattr_set
> ext4_xattr_set_handle
> ext4_xattr_ibody_find
> >> s->end < s->base
> >> no EXT4_STATE_XATTR
> >> xattr_check_inode is not executed
> ext4_xattr_ibody_set
> ext4_xattr_set_entry
> >> size_t min_offs = s->end - s->base
> >> UAF in memcpy
>
> we can easily reproduce this problem with the following commands:
> mkfs.ext4 -F /dev/sda
> mount -o debug_want_extra_isize=128 /dev/sda /mnt
> touch /mnt/file
> setfattr -n user.cat -v `seq -s z 4096|tr -d '[:digit:]'` /mnt/file
Thanks for sharing the test case. Indeed this results into UAF.
>
> In ext4_xattr_ibody_find, we have the following assignment logic:
> header = IHDR(inode, raw_inode)
> = raw_inode + EXT4_GOOD_OLD_INODE_SIZE + i_extra_isize
> is->s.base = IFIRST(header)
> = header + sizeof(struct ext4_xattr_ibody_header)
> is->s.end = raw_inode + s_inode_size
>
> In ext4_xattr_set_entry
> min_offs = s->end - s->base
> = s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
> sizeof(struct ext4_xattr_ibody_header)
> last = s->first
> free = min_offs - ((void *)last - s->base) - sizeof(__u32)
> = s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
> sizeof(struct ext4_xattr_ibody_header) - sizeof(__u32)
>
> In the calculation formula, all values except s_inode_size and
> i_extra_size are fixed values. When i_extra_size is the maximum value
> s_inode_size - EXT4_GOOD_OLD_INODE_SIZE, min_offs is -4 and free is -8.
> The value overflows. As a result, the preceding issue is triggered when
> memcpy is executed.
>
> Therefore, when finding xattr or setting xattr, check whether
> there is space for storing xattr in the inode to resolve this issue.
Sounds right. Thanks for fixing it and providing detailed analysis.
Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/xattr.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 042325349098..c3c3194f3ee1 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2176,8 +2176,9 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
> struct ext4_inode *raw_inode;
> int error;
>
> - if (EXT4_I(inode)->i_extra_isize == 0)
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> return 0;
> +
> raw_inode = ext4_raw_inode(&is->iloc);
> header = IHDR(inode, raw_inode);
> is->s.base = is->s.first = IFIRST(header);
> @@ -2205,8 +2206,9 @@ int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
> struct ext4_xattr_search *s = &is->s;
> int error;
>
> - if (EXT4_I(inode)->i_extra_isize == 0)
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> return -ENOSPC;
> +
> error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
> if (error)
> return error;
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry
2022-06-16 2:13 ` [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
2022-06-16 4:02 ` Ritesh Harjani
@ 2022-06-16 10:07 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-06-16 10:07 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list, lczerner,
enwlinux, linux-kernel, yi.zhang, yebin10, yukuai3, Hulk Robot
On Thu 16-06-22 10:13:56, Baokun Li wrote:
> Hulk Robot reported a issue:
> ==================================================================
> BUG: KASAN: use-after-free in ext4_xattr_set_entry+0x18ab/0x3500
> Write of size 4105 at addr ffff8881675ef5f4 by task syz-executor.0/7092
>
> CPU: 1 PID: 7092 Comm: syz-executor.0 Not tainted 4.19.90-dirty #17
> Call Trace:
> [...]
> memcpy+0x34/0x50 mm/kasan/kasan.c:303
> ext4_xattr_set_entry+0x18ab/0x3500 fs/ext4/xattr.c:1747
> ext4_xattr_ibody_inline_set+0x86/0x2a0 fs/ext4/xattr.c:2205
> ext4_xattr_set_handle+0x940/0x1300 fs/ext4/xattr.c:2386
> ext4_xattr_set+0x1da/0x300 fs/ext4/xattr.c:2498
> __vfs_setxattr+0x112/0x170 fs/xattr.c:149
> __vfs_setxattr_noperm+0x11b/0x2a0 fs/xattr.c:180
> __vfs_setxattr_locked+0x17b/0x250 fs/xattr.c:238
> vfs_setxattr+0xed/0x270 fs/xattr.c:255
> setxattr+0x235/0x330 fs/xattr.c:520
> path_setxattr+0x176/0x190 fs/xattr.c:539
> __do_sys_lsetxattr fs/xattr.c:561 [inline]
> __se_sys_lsetxattr fs/xattr.c:557 [inline]
> __x64_sys_lsetxattr+0xc2/0x160 fs/xattr.c:557
> do_syscall_64+0xdf/0x530 arch/x86/entry/common.c:298
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x459fe9
> RSP: 002b:00007fa5e54b4c08 EFLAGS: 00000246 ORIG_RAX: 00000000000000bd
> RAX: ffffffffffffffda RBX: 000000000051bf60 RCX: 0000000000459fe9
> RDX: 00000000200003c0 RSI: 0000000020000180 RDI: 0000000020000140
> RBP: 000000000051bf60 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000001009 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007ffc73c93fc0 R14: 000000000051bf60 R15: 00007fa5e54b4d80
> [...]
> ==================================================================
>
> Above issue may happen as follows:
> -------------------------------------
> ext4_xattr_set
> ext4_xattr_set_handle
> ext4_xattr_ibody_find
> >> s->end < s->base
> >> no EXT4_STATE_XATTR
> >> xattr_check_inode is not executed
> ext4_xattr_ibody_set
> ext4_xattr_set_entry
> >> size_t min_offs = s->end - s->base
> >> UAF in memcpy
>
> we can easily reproduce this problem with the following commands:
> mkfs.ext4 -F /dev/sda
> mount -o debug_want_extra_isize=128 /dev/sda /mnt
> touch /mnt/file
> setfattr -n user.cat -v `seq -s z 4096|tr -d '[:digit:]'` /mnt/file
>
> In ext4_xattr_ibody_find, we have the following assignment logic:
> header = IHDR(inode, raw_inode)
> = raw_inode + EXT4_GOOD_OLD_INODE_SIZE + i_extra_isize
> is->s.base = IFIRST(header)
> = header + sizeof(struct ext4_xattr_ibody_header)
> is->s.end = raw_inode + s_inode_size
>
> In ext4_xattr_set_entry
> min_offs = s->end - s->base
> = s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
> sizeof(struct ext4_xattr_ibody_header)
> last = s->first
> free = min_offs - ((void *)last - s->base) - sizeof(__u32)
> = s_inode_size - EXT4_GOOD_OLD_INODE_SIZE - i_extra_isize -
> sizeof(struct ext4_xattr_ibody_header) - sizeof(__u32)
>
> In the calculation formula, all values except s_inode_size and
> i_extra_size are fixed values. When i_extra_size is the maximum value
> s_inode_size - EXT4_GOOD_OLD_INODE_SIZE, min_offs is -4 and free is -8.
> The value overflows. As a result, the preceding issue is triggered when
> memcpy is executed.
>
> Therefore, when finding xattr or setting xattr, check whether
> there is space for storing xattr in the inode to resolve this issue.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Thanks for the fix! The patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/xattr.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 042325349098..c3c3194f3ee1 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2176,8 +2176,9 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
> struct ext4_inode *raw_inode;
> int error;
>
> - if (EXT4_I(inode)->i_extra_isize == 0)
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> return 0;
> +
> raw_inode = ext4_raw_inode(&is->iloc);
> header = IHDR(inode, raw_inode);
> is->s.base = is->s.first = IFIRST(header);
> @@ -2205,8 +2206,9 @@ int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
> struct ext4_xattr_search *s = &is->s;
> int error;
>
> - if (EXT4_I(inode)->i_extra_isize == 0)
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> return -ENOSPC;
> +
> error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
> if (error)
> return error;
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
2022-06-16 2:13 ` [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h Baokun Li
2022-06-16 2:13 ` [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
@ 2022-06-16 2:13 ` Baokun Li
2022-06-16 4:04 ` Ritesh Harjani
2022-06-16 10:08 ` Jan Kara
2022-06-16 2:13 ` [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode Baokun Li
` (2 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-06-16 2:13 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3, libaokun1
If the ext4 inode does not have xattr space, 0 is returned in the
get_max_inline_xattr_value_size function. Otherwise, the function returns
a negative value when the inode does not contain EXT4_STATE_XATTR.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/inline.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index cff52ff6549d..da5de43623dd 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -35,6 +35,9 @@ static int get_max_inline_xattr_value_size(struct inode *inode,
struct ext4_inode *raw_inode;
int free, min_offs;
+ if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
+ return 0;
+
min_offs = EXT4_SB(inode->i_sb)->s_inode_size -
EXT4_GOOD_OLD_INODE_SIZE -
EXT4_I(inode)->i_extra_isize -
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing
2022-06-16 2:13 ` [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing Baokun Li
@ 2022-06-16 4:04 ` Ritesh Harjani
2022-06-16 10:08 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Ritesh Harjani @ 2022-06-16 4:04 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3
On 22/06/16 10:13AM, Baokun Li wrote:
> If the ext4 inode does not have xattr space, 0 is returned in the
> get_max_inline_xattr_value_size function. Otherwise, the function returns
> a negative value when the inode does not contain EXT4_STATE_XATTR.
Yes, this looks good to me.
Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/inline.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index cff52ff6549d..da5de43623dd 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -35,6 +35,9 @@ static int get_max_inline_xattr_value_size(struct inode *inode,
> struct ext4_inode *raw_inode;
> int free, min_offs;
>
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> + return 0;
> +
> min_offs = EXT4_SB(inode->i_sb)->s_inode_size -
> EXT4_GOOD_OLD_INODE_SIZE -
> EXT4_I(inode)->i_extra_isize -
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing
2022-06-16 2:13 ` [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing Baokun Li
2022-06-16 4:04 ` Ritesh Harjani
@ 2022-06-16 10:08 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-06-16 10:08 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list, lczerner,
enwlinux, linux-kernel, yi.zhang, yebin10, yukuai3
On Thu 16-06-22 10:13:57, Baokun Li wrote:
> If the ext4 inode does not have xattr space, 0 is returned in the
> get_max_inline_xattr_value_size function. Otherwise, the function returns
> a negative value when the inode does not contain EXT4_STATE_XATTR.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/inline.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index cff52ff6549d..da5de43623dd 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -35,6 +35,9 @@ static int get_max_inline_xattr_value_size(struct inode *inode,
> struct ext4_inode *raw_inode;
> int free, min_offs;
>
> + if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
> + return 0;
> +
> min_offs = EXT4_SB(inode->i_sb)->s_inode_size -
> EXT4_GOOD_OLD_INODE_SIZE -
> EXT4_I(inode)->i_extra_isize -
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
` (2 preceding siblings ...)
2022-06-16 2:13 ` [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing Baokun Li
@ 2022-06-16 2:13 ` Baokun Li
2022-06-16 4:08 ` Ritesh Harjani
2022-06-16 10:09 ` Jan Kara
2022-07-14 15:00 ` [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Theodore Ts'o
2022-07-22 13:58 ` Theodore Ts'o
5 siblings, 2 replies; 15+ messages in thread
From: Baokun Li @ 2022-06-16 2:13 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3, libaokun1
Use the EXT4_INODE_HAS_XATTR_SPACE macro to more accurately
determine whether the inode have xattr space.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/inode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 53877ffe3c41..ae463cd9b405 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4687,8 +4687,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode,
__le32 *magic = (void *)raw_inode +
EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize;
- if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize + sizeof(__le32) <=
- EXT4_INODE_SIZE(inode->i_sb) &&
+ if (EXT4_INODE_HAS_XATTR_SPACE(inode) &&
*magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
ext4_set_inode_state(inode, EXT4_STATE_XATTR);
return ext4_find_inline_data_nolock(inode);
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode
2022-06-16 2:13 ` [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode Baokun Li
@ 2022-06-16 4:08 ` Ritesh Harjani
2022-06-16 10:09 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Ritesh Harjani @ 2022-06-16 4:08 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, lczerner, enwlinux,
linux-kernel, yi.zhang, yebin10, yukuai3
On 22/06/16 10:13AM, Baokun Li wrote:
> Use the EXT4_INODE_HAS_XATTR_SPACE macro to more accurately
> determine whether the inode have xattr space.
Right. I also noticed there are few places in fs/ext4/xattr.c
and in fs/ext4/inline.c where sizeof(__u32) is being used which (I think)
should be EXT4_XATTR_PAD. But that need not be part of this patch series.
Looks good to me. Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/inode.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 53877ffe3c41..ae463cd9b405 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4687,8 +4687,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode,
> __le32 *magic = (void *)raw_inode +
> EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize;
>
> - if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize + sizeof(__le32) <=
> - EXT4_INODE_SIZE(inode->i_sb) &&
> + if (EXT4_INODE_HAS_XATTR_SPACE(inode) &&
> *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
> ext4_set_inode_state(inode, EXT4_STATE_XATTR);
> return ext4_find_inline_data_nolock(inode);
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode
2022-06-16 2:13 ` [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode Baokun Li
2022-06-16 4:08 ` Ritesh Harjani
@ 2022-06-16 10:09 ` Jan Kara
1 sibling, 0 replies; 15+ messages in thread
From: Jan Kara @ 2022-06-16 10:09 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list, lczerner,
enwlinux, linux-kernel, yi.zhang, yebin10, yukuai3
On Thu 16-06-22 10:13:58, Baokun Li wrote:
> Use the EXT4_INODE_HAS_XATTR_SPACE macro to more accurately
> determine whether the inode have xattr space.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good! Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/inode.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 53877ffe3c41..ae463cd9b405 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4687,8 +4687,7 @@ static inline int ext4_iget_extra_inode(struct inode *inode,
> __le32 *magic = (void *)raw_inode +
> EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize;
>
> - if (EXT4_GOOD_OLD_INODE_SIZE + ei->i_extra_isize + sizeof(__le32) <=
> - EXT4_INODE_SIZE(inode->i_sb) &&
> + if (EXT4_INODE_HAS_XATTR_SPACE(inode) &&
> *magic == cpu_to_le32(EXT4_XATTR_MAGIC)) {
> ext4_set_inode_state(inode, EXT4_STATE_XATTR);
> return ext4_find_inline_data_nolock(inode);
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
` (3 preceding siblings ...)
2022-06-16 2:13 ` [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode Baokun Li
@ 2022-07-14 15:00 ` Theodore Ts'o
2022-07-22 13:58 ` Theodore Ts'o
5 siblings, 0 replies; 15+ messages in thread
From: Theodore Ts'o @ 2022-07-14 15:00 UTC (permalink / raw)
To: linux-ext4, libaokun1
Cc: Theodore Ts'o, jack, yi.zhang, linux-kernel, enwlinux,
yebin10, lczerner, ritesh.list, adilger.kernel, yukuai3
On Thu, 16 Jun 2022 10:13:54 +0800, Baokun Li wrote:
> This series adds a macro for whether there is space for xattr in
> ext4 inode, and fixes some problems with this macro.
>
> V1->V2:
> Split the patch to make the logic clearer.
> Rename macro to EXT4_INODE_HAVE_XATTR_SPACE.
> V2->V3:
> Rename macro to EXT4_INODE_HAS_XATTR_SPACE.
>
> [...]
Applied, thanks!
[1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
commit: ff528f6b155ce79adf38583a66867d8e54cbd460
[2/4] ext4: fix use-after-free in ext4_xattr_set_entry
commit: 0847102f2b38b43f7352ed8a7f714a291ed1513d
[3/4] ext4: correct max_inline_xattr_value_size computing
commit: 3d783a3751995003002a5f4f6d333c7c02c7966e
[4/4] ext4: correct the misjudgment in ext4_iget_extra_inode
commit: 31c5d92b53629452d669980d17adbd22f2af0d26
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry
2022-06-16 2:13 [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Baokun Li
` (4 preceding siblings ...)
2022-07-14 15:00 ` [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry Theodore Ts'o
@ 2022-07-22 13:58 ` Theodore Ts'o
5 siblings, 0 replies; 15+ messages in thread
From: Theodore Ts'o @ 2022-07-22 13:58 UTC (permalink / raw)
To: libaokun1, linux-ext4
Cc: Theodore Ts'o, jack, lczerner, yebin10, yi.zhang,
ritesh.list, linux-kernel, enwlinux, adilger.kernel, yukuai3
On Thu, 16 Jun 2022 10:13:54 +0800, Baokun Li wrote:
> This series adds a macro for whether there is space for xattr in
> ext4 inode, and fixes some problems with this macro.
>
> V1->V2:
> Split the patch to make the logic clearer.
> Rename macro to EXT4_INODE_HAVE_XATTR_SPACE.
> V2->V3:
> Rename macro to EXT4_INODE_HAS_XATTR_SPACE.
>
> [...]
Applied, thanks!
[1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
commit: ff528f6b155ce79adf38583a66867d8e54cbd460
[2/4] ext4: fix use-after-free in ext4_xattr_set_entry
commit: 0847102f2b38b43f7352ed8a7f714a291ed1513d
[3/4] ext4: correct max_inline_xattr_value_size computing
commit: 3d783a3751995003002a5f4f6d333c7c02c7966e
[4/4] ext4: correct the misjudgment in ext4_iget_extra_inode
commit: 31c5d92b53629452d669980d17adbd22f2af0d26
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 15+ messages in thread