[PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry

[PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Baokun Li]

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.

Baokun Li (4):
  ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h
  ext4: fix use-after-free in ext4_xattr_set_entry
  ext4: correct max_inline_xattr_value_size computing
  ext4: correct the misjudgment in ext4_iget_extra_inode

 fs/ext4/inline.c |  3 +++
 fs/ext4/inode.c  |  3 +--
 fs/ext4/xattr.c  |  6 ++++--
 fs/ext4/xattr.h  | 13 +++++++++++++
 4 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h

[Baokun Li]

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

[PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Baokun Li]

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

[PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing

[Baokun Li]

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

[PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode

[Baokun Li]

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

Re: [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h

[Ritesh Harjani]

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
>

Re: [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Ritesh Harjani]

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
>

Re: [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing

[Ritesh Harjani]

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
>

Re: [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode

[Ritesh Harjani]

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
>

Re: [PATCH v3 1/4] ext4: add EXT4_INODE_HAS_XATTR_SPACE macro in xattr.h

[Jan Kara]

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

Re: [PATCH v3 2/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Jan Kara]

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

Re: [PATCH v3 3/4] ext4: correct max_inline_xattr_value_size computing

[Jan Kara]

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

Re: [PATCH v3 4/4] ext4: correct the misjudgment in ext4_iget_extra_inode

[Jan Kara]

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

Re: [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Theodore Ts'o]

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>

Re: [PATCH v3 0/4] ext4: fix use-after-free in ext4_xattr_set_entry

[Theodore Ts'o]

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>