* [PATCH][for stable [4.14, 5.10] 1/3] Revert "ext4: fix use-after-free in ext4_xattr_set_entry"
2023-04-19 6:46 [PATCH][for stable [4.14, 5.10] 0/3] ext4: fix use-after-free in ext4_xattr_set_entry Tudor Ambarus
@ 2023-04-19 6:46 ` Tudor Ambarus
2023-04-19 6:46 ` [PATCH][for stable [4.14, 5.10] 2/3] ext4: remove duplicate definition of ext4_xattr_ibody_inline_set() Tudor Ambarus
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Tudor Ambarus @ 2023-04-19 6:46 UTC (permalink / raw)
To: stable
Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, joneslee,
Tudor Ambarus
This reverts commit bb8592efcf8ef2f62947745d3182ea05b5256a15 which is
commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3 upstream.
The order in which patches are queued to stable matters. This patch
has a logical dependency on commit 310c097c2bdbea253d6ee4e064f3e65580ef93ac
upstream, and failing to queue the latter results in a null-ptr-deref
reported at the Link below.
In order to avoid conflicts on stable, revert the commit just so that we
can queue its prerequisite patch first and then queue the same after.
Link: https://syzkaller.appspot.com/bug?extid=d5ebf56f3b1268136afd
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
fs/ext4/xattr.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index f3da1f2d4cb9..948da799abab 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2193,9 +2193,8 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
struct ext4_inode *raw_inode;
int error;
- if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
+ if (EXT4_I(inode)->i_extra_isize == 0)
return 0;
-
raw_inode = ext4_raw_inode(&is->iloc);
header = IHDR(inode, raw_inode);
is->s.base = is->s.first = IFIRST(header);
@@ -2223,9 +2222,8 @@ int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
struct ext4_xattr_search *s = &is->s;
int error;
- if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
+ if (EXT4_I(inode)->i_extra_isize == 0)
return -ENOSPC;
-
error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
if (error)
return error;
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH][for stable [4.14, 5.10] 2/3] ext4: remove duplicate definition of ext4_xattr_ibody_inline_set()
2023-04-19 6:46 [PATCH][for stable [4.14, 5.10] 0/3] ext4: fix use-after-free in ext4_xattr_set_entry Tudor Ambarus
2023-04-19 6:46 ` [PATCH][for stable [4.14, 5.10] 1/3] Revert "ext4: fix use-after-free in ext4_xattr_set_entry" Tudor Ambarus
@ 2023-04-19 6:46 ` Tudor Ambarus
2023-04-19 6:46 ` [PATCH][for stable [4.14, 5.10] 3/3] ext4: fix use-after-free in ext4_xattr_set_entry Tudor Ambarus
2023-04-23 13:22 ` [PATCH][for stable [4.14, 5.10] 0/3] " Greg KH
3 siblings, 0 replies; 5+ messages in thread
From: Tudor Ambarus @ 2023-04-19 6:46 UTC (permalink / raw)
To: stable
Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, joneslee,
Ritesh Harjani, Tudor Ambarus
From: Ritesh Harjani <riteshh@linux.ibm.com>
[ Upstream commit 310c097c2bdbea253d6ee4e064f3e65580ef93ac ]
ext4_xattr_ibody_inline_set() & ext4_xattr_ibody_set() have the exact
same definition. Hence remove ext4_xattr_ibody_inline_set() and all
its call references. Convert the callers of it to call
ext4_xattr_ibody_set() instead.
[ Modified to preserve ext4_xattr_ibody_set() and remove
ext4_xattr_ibody_inline_set() instead. -- TYT ]
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Link: https://lore.kernel.org/r/fd566b799bbbbe9b668eb5eecde5b5e319e3694f.1622685482.git.riteshh@linux.ibm.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
fs/ext4/inline.c | 11 +++++------
fs/ext4/xattr.c | 26 +-------------------------
fs/ext4/xattr.h | 6 +++---
3 files changed, 9 insertions(+), 34 deletions(-)
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 61cb50e8fcb7..0758f606f006 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -206,7 +206,7 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
/*
* write the buffer to the inline inode.
* If 'create' is set, we don't need to do the extra copy in the xattr
- * value since it is already handled by ext4_xattr_ibody_inline_set.
+ * value since it is already handled by ext4_xattr_ibody_set.
* That saves us one memcpy.
*/
static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
@@ -288,7 +288,7 @@ static int ext4_create_inline_data(handle_t *handle,
BUG_ON(!is.s.not_found);
- error = ext4_xattr_ibody_inline_set(handle, inode, &i, &is);
+ error = ext4_xattr_ibody_set(handle, inode, &i, &is);
if (error) {
if (error == -ENOSPC)
ext4_clear_inode_state(inode,
@@ -360,7 +360,7 @@ static int ext4_update_inline_data(handle_t *handle, struct inode *inode,
i.value = value;
i.value_len = len;
- error = ext4_xattr_ibody_inline_set(handle, inode, &i, &is);
+ error = ext4_xattr_ibody_set(handle, inode, &i, &is);
if (error)
goto out;
@@ -433,7 +433,7 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,
if (error)
goto out;
- error = ext4_xattr_ibody_inline_set(handle, inode, &i, &is);
+ error = ext4_xattr_ibody_set(handle, inode, &i, &is);
if (error)
goto out;
@@ -1930,8 +1930,7 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
i.value = value;
i.value_len = i_size > EXT4_MIN_INLINE_DATA_SIZE ?
i_size - EXT4_MIN_INLINE_DATA_SIZE : 0;
- err = ext4_xattr_ibody_inline_set(handle, inode,
- &i, &is);
+ err = ext4_xattr_ibody_set(handle, inode, &i, &is);
if (err)
goto out_error;
}
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 948da799abab..71e83e815258 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2214,31 +2214,7 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
return 0;
}
-int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
- struct ext4_xattr_info *i,
- struct ext4_xattr_ibody_find *is)
-{
- struct ext4_xattr_ibody_header *header;
- struct ext4_xattr_search *s = &is->s;
- int error;
-
- if (EXT4_I(inode)->i_extra_isize == 0)
- return -ENOSPC;
- error = ext4_xattr_set_entry(i, s, handle, inode, false /* is_block */);
- if (error)
- return error;
- header = IHDR(inode, ext4_raw_inode(&is->iloc));
- if (!IS_LAST_ENTRY(s->first)) {
- header->h_magic = cpu_to_le32(EXT4_XATTR_MAGIC);
- ext4_set_inode_state(inode, EXT4_STATE_XATTR);
- } else {
- header->h_magic = cpu_to_le32(0);
- ext4_clear_inode_state(inode, EXT4_STATE_XATTR);
- }
- return 0;
-}
-
-static int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
+int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
struct ext4_xattr_info *i,
struct ext4_xattr_ibody_find *is)
{
diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index b357872ab83b..e5e36bd11f05 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -200,9 +200,9 @@ extern int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
extern int ext4_xattr_ibody_get(struct inode *inode, int name_index,
const char *name,
void *buffer, size_t buffer_size);
-extern int ext4_xattr_ibody_inline_set(handle_t *handle, struct inode *inode,
- struct ext4_xattr_info *i,
- struct ext4_xattr_ibody_find *is);
+extern int ext4_xattr_ibody_set(handle_t *handle, struct inode *inode,
+ struct ext4_xattr_info *i,
+ struct ext4_xattr_ibody_find *is);
extern struct mb_cache *ext4_xattr_create_cache(void);
extern void ext4_xattr_destroy_cache(struct mb_cache *);
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH][for stable [4.14, 5.10] 3/3] ext4: fix use-after-free in ext4_xattr_set_entry
2023-04-19 6:46 [PATCH][for stable [4.14, 5.10] 0/3] ext4: fix use-after-free in ext4_xattr_set_entry Tudor Ambarus
2023-04-19 6:46 ` [PATCH][for stable [4.14, 5.10] 1/3] Revert "ext4: fix use-after-free in ext4_xattr_set_entry" Tudor Ambarus
2023-04-19 6:46 ` [PATCH][for stable [4.14, 5.10] 2/3] ext4: remove duplicate definition of ext4_xattr_ibody_inline_set() Tudor Ambarus
@ 2023-04-19 6:46 ` Tudor Ambarus
2023-04-23 13:22 ` [PATCH][for stable [4.14, 5.10] 0/3] " Greg KH
3 siblings, 0 replies; 5+ messages in thread
From: Tudor Ambarus @ 2023-04-19 6:46 UTC (permalink / raw)
To: stable
Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, joneslee,
Baokun Li, stable, Hulk Robot, Ritesh Harjani, Jan Kara,
Tudor Ambarus
From: Baokun Li <libaokun1@huawei.com>
[ Upstream commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3 ]
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.
Cc: stable@kernel.org
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20220616021358.2504451-3-libaokun1@huawei.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
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 71e83e815258..28fa9a64dc4b 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2193,8 +2193,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);
@@ -2222,8 +2223,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.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread