When we traverse xattr entries via __find_xattr(),
if the raw filesystem content is faked or any hardware failure occurs,
out-of-bound error can be detected by KASAN.
Fix the issue by introducing boundary check.
[ 38.402878] c7 1827 BUG: KASAN: slab-out-of-bounds in f2fs_getxattr+0x518/0x68c
[ 38.402891] c7 1827 Read of size 4 at addr ffffffc0b6fb35dc by task
[ 38.402935] c7 1827 Call trace:
[ 38.402952] c7 1827 [<ffffff900809003c>] dump_backtrace+0x0/0x6bc
[ 38.402966] c7 1827 [<ffffff9008090030>] show_stack+0x20/0x2c
[ 38.402981] c7 1827 [<ffffff900871ab10>] dump_stack+0xfc/0x140
[ 38.402995] c7 1827 [<ffffff9008325c40>] print_address_description+0x80/0x2d8
[ 38.403009] c7 1827 [<ffffff900832629c>] kasan_report_error+0x198/0x1fc
[ 38.403022] c7 1827 [<ffffff9008326104>] kasan_report_error+0x0/0x1fc
[ 38.403037] c7 1827 [<ffffff9008325000>] __asan_load4+0x1b0/0x1b8
[ 38.403051] c7 1827 [<ffffff90085fcc44>] f2fs_getxattr+0x518/0x68c
[ 38.403066] c7 1827 [<ffffff90085fc508>] f2fs_xattr_generic_get+0xb0/0xd0
[ 38.403080] c7 1827 [<ffffff9008395708>] __vfs_getxattr+0x1f4/0x1fc
[ 38.403096] c7 1827 [<ffffff9008621bd0>] inode_doinit_with_dentry+0x360/0x938
[ 38.403109] c7 1827 [<ffffff900862d6cc>] selinux_d_instantiate+0x2c/0x38
[ 38.403123] c7 1827 [<ffffff900861b018>] security_d_instantiate+0x68/0x98
[ 38.403136] c7 1827 [<ffffff9008377db8>] d_splice_alias+0x58/0x348
[ 38.403149] c7 1827 [<ffffff900858d16c>] f2fs_lookup+0x608/0x774
[ 38.403163] c7 1827 [<ffffff900835eacc>] lookup_slow+0x1e0/0x2cc
[ 38.403177] c7 1827 [<ffffff9008367fe0>] walk_component+0x160/0x520
[ 38.403190] c7 1827 [<ffffff9008369ef4>] path_lookupat+0x110/0x2b4
[ 38.403203] c7 1827 [<ffffff900835dd38>] filename_lookup+0x1d8/0x3a8
[ 38.403216] c7 1827 [<ffffff900835eeb0>] user_path_at_empty+0x54/0x68
[ 38.403229] c7 1827 [<ffffff9008395f44>] SyS_getxattr+0xb4/0x18c
[ 38.403241] c7 1827 [<ffffff9008084200>] el0_svc_naked+0x34/0x38
Bug: 126558260
Signed-off-by: Randall Huang <[email protected]>
---
fs/f2fs/xattr.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 848a785abe25..0531c1e38275 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -202,12 +202,17 @@ static inline const struct xattr_handler *f2fs_xattr_handler(int index)
return handler;
}
-static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
- size_t len, const char *name)
+static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
+ int base_addr_limit, int index,
+ size_t len, const char *name)
{
struct f2fs_xattr_entry *entry;
+ void *max_addr = base_addr + ENTRY_SIZE(XATTR_ENTRY(base_addr)) +
+ base_addr_limit - 1;
list_for_each_xattr(entry, base_addr) {
+ if ((void *)entry + sizeof(__u32) > max_addr)
+ return NULL;
if (entry->e_name_index != index)
continue;
if (entry->e_name_len != len)
@@ -337,9 +342,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
else
cur_addr = txattr_addr;
- *xe = __find_xattr(cur_addr, index, len, name);
+ *xe = __find_xattr(cur_addr, XATTR_PADDING_SIZE, index, len, name);
check:
- if (IS_XATTR_LAST_ENTRY(*xe)) {
+ if (!*xe || IS_XATTR_LAST_ENTRY(*xe)) {
err = -ENODATA;
goto out;
}
@@ -606,9 +611,14 @@ static int __f2fs_setxattr(struct inode *inode, int index,
return error;
/* find entry with wanted name. */
- here = __find_xattr(base_addr, index, len, name);
+ here = __find_xattr(base_addr, inline_xattr_size(inode) +
+ VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE,
+ index, len, name);
- found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
+ if (!here)
+ found = 0;
+ else
+ found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
if (found) {
if ((flags & XATTR_CREATE)) {
--
2.21.0.392.gf8f6787159e-goog
Hi Randall,
On 2019/4/8 16:50, Randall Huang wrote:
> When we traverse xattr entries via __find_xattr(),
> if the raw filesystem content is faked or any hardware failure occurs,
> out-of-bound error can be detected by KASAN.
> Fix the issue by introducing boundary check.
>
> [ 38.402878] c7 1827 BUG: KASAN: slab-out-of-bounds in f2fs_getxattr+0x518/0x68c
> [ 38.402891] c7 1827 Read of size 4 at addr ffffffc0b6fb35dc by task
> [ 38.402935] c7 1827 Call trace:
> [ 38.402952] c7 1827 [<ffffff900809003c>] dump_backtrace+0x0/0x6bc
> [ 38.402966] c7 1827 [<ffffff9008090030>] show_stack+0x20/0x2c
> [ 38.402981] c7 1827 [<ffffff900871ab10>] dump_stack+0xfc/0x140
> [ 38.402995] c7 1827 [<ffffff9008325c40>] print_address_description+0x80/0x2d8
> [ 38.403009] c7 1827 [<ffffff900832629c>] kasan_report_error+0x198/0x1fc
> [ 38.403022] c7 1827 [<ffffff9008326104>] kasan_report_error+0x0/0x1fc
> [ 38.403037] c7 1827 [<ffffff9008325000>] __asan_load4+0x1b0/0x1b8
> [ 38.403051] c7 1827 [<ffffff90085fcc44>] f2fs_getxattr+0x518/0x68c
> [ 38.403066] c7 1827 [<ffffff90085fc508>] f2fs_xattr_generic_get+0xb0/0xd0
> [ 38.403080] c7 1827 [<ffffff9008395708>] __vfs_getxattr+0x1f4/0x1fc
> [ 38.403096] c7 1827 [<ffffff9008621bd0>] inode_doinit_with_dentry+0x360/0x938
> [ 38.403109] c7 1827 [<ffffff900862d6cc>] selinux_d_instantiate+0x2c/0x38
> [ 38.403123] c7 1827 [<ffffff900861b018>] security_d_instantiate+0x68/0x98
> [ 38.403136] c7 1827 [<ffffff9008377db8>] d_splice_alias+0x58/0x348
> [ 38.403149] c7 1827 [<ffffff900858d16c>] f2fs_lookup+0x608/0x774
> [ 38.403163] c7 1827 [<ffffff900835eacc>] lookup_slow+0x1e0/0x2cc
> [ 38.403177] c7 1827 [<ffffff9008367fe0>] walk_component+0x160/0x520
> [ 38.403190] c7 1827 [<ffffff9008369ef4>] path_lookupat+0x110/0x2b4
> [ 38.403203] c7 1827 [<ffffff900835dd38>] filename_lookup+0x1d8/0x3a8
> [ 38.403216] c7 1827 [<ffffff900835eeb0>] user_path_at_empty+0x54/0x68
> [ 38.403229] c7 1827 [<ffffff9008395f44>] SyS_getxattr+0xb4/0x18c
> [ 38.403241] c7 1827 [<ffffff9008084200>] el0_svc_naked+0x34/0x38
>
> Bug: 126558260
>
> Signed-off-by: Randall Huang <[email protected]>
> ---
> fs/f2fs/xattr.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 848a785abe25..0531c1e38275 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -202,12 +202,17 @@ static inline const struct xattr_handler *f2fs_xattr_handler(int index)
> return handler;
> }
>
> -static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
> - size_t len, const char *name)
> +static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
> + int base_addr_limit, int index,
unsigned int max_size,
> + size_t len, const char *name)
> {
> struct f2fs_xattr_entry *entry;
> + void *max_addr = base_addr + ENTRY_SIZE(XATTR_ENTRY(base_addr)) +
> + base_addr_limit - 1;
If I'm not missing something, shouldn't it be?
void *max_addr = base_addr + max_size;
>
> list_for_each_xattr(entry, base_addr) {
> + if ((void *)entry + sizeof(__u32) > max_addr)
> + return NULL;
> if (entry->e_name_index != index)
> continue;
> if (entry->e_name_len != len)
> @@ -337,9 +342,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
> else
> cur_addr = txattr_addr;
>
> - *xe = __find_xattr(cur_addr, index, len, name);
> + *xe = __find_xattr(cur_addr, XATTR_PADDING_SIZE, index, len, name);
max_size = *base_size - (txattr_addr - cur_addr);
*xe = __find_xattr(cur_addr, max_size, index, len, name);
> check:
> - if (IS_XATTR_LAST_ENTRY(*xe)) {
> + if (!*xe || IS_XATTR_LAST_ENTRY(*xe)) {
If xattr entry across boundary of max xattr space size, maybe we'd better return -EFAULT
which can be distinguished from a real -ENODATA error, latter, we can set SBI_NEED_FSCK
to give a repairing hint to fsck. :)
> err = -ENODATA;
> goto out;
> }
> @@ -606,9 +611,14 @@ static int __f2fs_setxattr(struct inode *inode, int index,
> return error;
>
> /* find entry with wanted name. */
> - here = __find_xattr(base_addr, index, len, name);
> + here = __find_xattr(base_addr, inline_xattr_size(inode) +
> + VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE,
> + index, len, name);
unsigned int size = F2FS_I(inode)->i_xattr_nid ? VALID_XATTR_BLOCK_SIZE : 0;
unsigned int max_size = inline_xattr_size(inode) + size + XATTR_PADDING_SIZE;
here = __find_xattr(..., max_size, ...);
if (!here)
return -EFAULT;
Thanks,
>
> - found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
> + if (!here)
> + found = 0;
> + else
> + found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>
> if (found) {
> if ((flags & XATTR_CREATE)) {
>
On 2019/4/8 20:03, Chao Yu wrote:
> Hi Randall,
>
> On 2019/4/8 16:50, Randall Huang wrote:
>> When we traverse xattr entries via __find_xattr(),
>> if the raw filesystem content is faked or any hardware failure occurs,
>> out-of-bound error can be detected by KASAN.
>> Fix the issue by introducing boundary check.
>>
>> [ 38.402878] c7 1827 BUG: KASAN: slab-out-of-bounds in f2fs_getxattr+0x518/0x68c
>> [ 38.402891] c7 1827 Read of size 4 at addr ffffffc0b6fb35dc by task
>> [ 38.402935] c7 1827 Call trace:
>> [ 38.402952] c7 1827 [<ffffff900809003c>] dump_backtrace+0x0/0x6bc
>> [ 38.402966] c7 1827 [<ffffff9008090030>] show_stack+0x20/0x2c
>> [ 38.402981] c7 1827 [<ffffff900871ab10>] dump_stack+0xfc/0x140
>> [ 38.402995] c7 1827 [<ffffff9008325c40>] print_address_description+0x80/0x2d8
>> [ 38.403009] c7 1827 [<ffffff900832629c>] kasan_report_error+0x198/0x1fc
>> [ 38.403022] c7 1827 [<ffffff9008326104>] kasan_report_error+0x0/0x1fc
>> [ 38.403037] c7 1827 [<ffffff9008325000>] __asan_load4+0x1b0/0x1b8
>> [ 38.403051] c7 1827 [<ffffff90085fcc44>] f2fs_getxattr+0x518/0x68c
>> [ 38.403066] c7 1827 [<ffffff90085fc508>] f2fs_xattr_generic_get+0xb0/0xd0
>> [ 38.403080] c7 1827 [<ffffff9008395708>] __vfs_getxattr+0x1f4/0x1fc
>> [ 38.403096] c7 1827 [<ffffff9008621bd0>] inode_doinit_with_dentry+0x360/0x938
>> [ 38.403109] c7 1827 [<ffffff900862d6cc>] selinux_d_instantiate+0x2c/0x38
>> [ 38.403123] c7 1827 [<ffffff900861b018>] security_d_instantiate+0x68/0x98
>> [ 38.403136] c7 1827 [<ffffff9008377db8>] d_splice_alias+0x58/0x348
>> [ 38.403149] c7 1827 [<ffffff900858d16c>] f2fs_lookup+0x608/0x774
>> [ 38.403163] c7 1827 [<ffffff900835eacc>] lookup_slow+0x1e0/0x2cc
>> [ 38.403177] c7 1827 [<ffffff9008367fe0>] walk_component+0x160/0x520
>> [ 38.403190] c7 1827 [<ffffff9008369ef4>] path_lookupat+0x110/0x2b4
>> [ 38.403203] c7 1827 [<ffffff900835dd38>] filename_lookup+0x1d8/0x3a8
>> [ 38.403216] c7 1827 [<ffffff900835eeb0>] user_path_at_empty+0x54/0x68
>> [ 38.403229] c7 1827 [<ffffff9008395f44>] SyS_getxattr+0xb4/0x18c
>> [ 38.403241] c7 1827 [<ffffff9008084200>] el0_svc_naked+0x34/0x38
>>
>> Bug: 126558260
>>
>> Signed-off-by: Randall Huang <[email protected]>
>> ---
>> fs/f2fs/xattr.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>> index 848a785abe25..0531c1e38275 100644
>> --- a/fs/f2fs/xattr.c
>> +++ b/fs/f2fs/xattr.c
>> @@ -202,12 +202,17 @@ static inline const struct xattr_handler *f2fs_xattr_handler(int index)
>> return handler;
>> }
>>
>> -static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
>> - size_t len, const char *name)
>> +static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
>> + int base_addr_limit, int index,
>
> unsigned int max_size,
>
>> + size_t len, const char *name)
>> {
>> struct f2fs_xattr_entry *entry;
>> + void *max_addr = base_addr + ENTRY_SIZE(XATTR_ENTRY(base_addr)) +
>> + base_addr_limit - 1;
>
> If I'm not missing something, shouldn't it be?
>
> void *max_addr = base_addr + max_size;
>
>>
>> list_for_each_xattr(entry, base_addr) {
>> + if ((void *)entry + sizeof(__u32) > max_addr)
>> + return NULL;
>> if (entry->e_name_index != index)
>> continue;
>> if (entry->e_name_len != len)
>> @@ -337,9 +342,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
>> else
>> cur_addr = txattr_addr;
>>
>> - *xe = __find_xattr(cur_addr, index, len, name);
>> + *xe = __find_xattr(cur_addr, XATTR_PADDING_SIZE, index, len, name);
>
> max_size = *base_size - (txattr_addr - cur_addr);
max_size = *base_size - (cur_addr - txattr_addr);
> *xe = __find_xattr(cur_addr, max_size, index, len, name);
>
>> check:
>> - if (IS_XATTR_LAST_ENTRY(*xe)) {
>> + if (!*xe || IS_XATTR_LAST_ENTRY(*xe)) {
>
> If xattr entry across boundary of max xattr space size, maybe we'd better return -EFAULT
> which can be distinguished from a real -ENODATA error, latter, we can set SBI_NEED_FSCK
> to give a repairing hint to fsck. :)
>
>> err = -ENODATA;
>> goto out;
>> }
>> @@ -606,9 +611,14 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>> return error;
>>
>> /* find entry with wanted name. */
>> - here = __find_xattr(base_addr, index, len, name);
>> + here = __find_xattr(base_addr, inline_xattr_size(inode) +
>> + VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE,
>> + index, len, name);
>
> unsigned int size = F2FS_I(inode)->i_xattr_nid ? VALID_XATTR_BLOCK_SIZE : 0;
> unsigned int max_size = inline_xattr_size(inode) + size + XATTR_PADDING_SIZE;
>
> here = __find_xattr(..., max_size, ...);
>
> if (!here)
> return -EFAULT;
>
> Thanks,
>
>>
>> - found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>> + if (!here)
>> + found = 0;
>> + else
>> + found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>>
>> if (found) {
>> if ((flags & XATTR_CREATE)) {
>>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
On Mon, Apr 08, 2019 at 08:14:43PM +0800, Chao Yu wrote:
> On 2019/4/8 20:03, Chao Yu wrote:
> > Hi Randall,
> >
> > On 2019/4/8 16:50, Randall Huang wrote:
> >> When we traverse xattr entries via __find_xattr(),
> >> if the raw filesystem content is faked or any hardware failure occurs,
> >> out-of-bound error can be detected by KASAN.
> >> Fix the issue by introducing boundary check.
> >>
> >> [ 38.402878] c7 1827 BUG: KASAN: slab-out-of-bounds in f2fs_getxattr+0x518/0x68c
> >> [ 38.402891] c7 1827 Read of size 4 at addr ffffffc0b6fb35dc by task
> >> [ 38.402935] c7 1827 Call trace:
> >> [ 38.402952] c7 1827 [<ffffff900809003c>] dump_backtrace+0x0/0x6bc
> >> [ 38.402966] c7 1827 [<ffffff9008090030>] show_stack+0x20/0x2c
> >> [ 38.402981] c7 1827 [<ffffff900871ab10>] dump_stack+0xfc/0x140
> >> [ 38.402995] c7 1827 [<ffffff9008325c40>] print_address_description+0x80/0x2d8
> >> [ 38.403009] c7 1827 [<ffffff900832629c>] kasan_report_error+0x198/0x1fc
> >> [ 38.403022] c7 1827 [<ffffff9008326104>] kasan_report_error+0x0/0x1fc
> >> [ 38.403037] c7 1827 [<ffffff9008325000>] __asan_load4+0x1b0/0x1b8
> >> [ 38.403051] c7 1827 [<ffffff90085fcc44>] f2fs_getxattr+0x518/0x68c
> >> [ 38.403066] c7 1827 [<ffffff90085fc508>] f2fs_xattr_generic_get+0xb0/0xd0
> >> [ 38.403080] c7 1827 [<ffffff9008395708>] __vfs_getxattr+0x1f4/0x1fc
> >> [ 38.403096] c7 1827 [<ffffff9008621bd0>] inode_doinit_with_dentry+0x360/0x938
> >> [ 38.403109] c7 1827 [<ffffff900862d6cc>] selinux_d_instantiate+0x2c/0x38
> >> [ 38.403123] c7 1827 [<ffffff900861b018>] security_d_instantiate+0x68/0x98
> >> [ 38.403136] c7 1827 [<ffffff9008377db8>] d_splice_alias+0x58/0x348
> >> [ 38.403149] c7 1827 [<ffffff900858d16c>] f2fs_lookup+0x608/0x774
> >> [ 38.403163] c7 1827 [<ffffff900835eacc>] lookup_slow+0x1e0/0x2cc
> >> [ 38.403177] c7 1827 [<ffffff9008367fe0>] walk_component+0x160/0x520
> >> [ 38.403190] c7 1827 [<ffffff9008369ef4>] path_lookupat+0x110/0x2b4
> >> [ 38.403203] c7 1827 [<ffffff900835dd38>] filename_lookup+0x1d8/0x3a8
> >> [ 38.403216] c7 1827 [<ffffff900835eeb0>] user_path_at_empty+0x54/0x68
> >> [ 38.403229] c7 1827 [<ffffff9008395f44>] SyS_getxattr+0xb4/0x18c
> >> [ 38.403241] c7 1827 [<ffffff9008084200>] el0_svc_naked+0x34/0x38
> >>
> >> Bug: 126558260
> >>
> >> Signed-off-by: Randall Huang <[email protected]>
> >> ---
> >> fs/f2fs/xattr.c | 22 ++++++++++++++++------
> >> 1 file changed, 16 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> >> index 848a785abe25..0531c1e38275 100644
> >> --- a/fs/f2fs/xattr.c
> >> +++ b/fs/f2fs/xattr.c
> >> @@ -202,12 +202,17 @@ static inline const struct xattr_handler *f2fs_xattr_handler(int index)
> >> return handler;
> >> }
> >>
> >> -static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
> >> - size_t len, const char *name)
> >> +static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
> >> + int base_addr_limit, int index,
> >
> > unsigned int max_size,
> >
> >> + size_t len, const char *name)
> >> {
> >> struct f2fs_xattr_entry *entry;
> >> + void *max_addr = base_addr + ENTRY_SIZE(XATTR_ENTRY(base_addr)) +
> >> + base_addr_limit - 1;
> >
> > If I'm not missing something, shouldn't it be?
> >
> > void *max_addr = base_addr + max_size;
> >
Hi Chao,
Let me show the buggy example of xattr entries.
Here is the buggy xattr entries.
The address 0x1201f24 - 0x1201fcb is correct content.
(Each entry occupies 4 + 9 + 10 + 1 bytes, 24 bytes)
Hacker append a faked entry at 0x1201fcc - 0x1201fe5
(entry length = 4 + 9 + 12 + 1 bytes)
01201f00 00 00 00 00 00 00 00 00 00 00 00 00 11 20 f5 f2 |............. ..|
01201f10 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
01201f20 00 00 00 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |........attrname|
01201f30 31 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |1attrvalue1.....|
01201f40 61 74 74 72 6e 61 6d 65 32 61 74 74 72 76 61 6c |attrname2attrval|
01201f50 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
01201f60 33 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |3attrvalue1.....|
01201f70 61 74 74 72 6e 61 6d 65 34 61 74 74 72 76 61 6c |attrname4attrval|
01201f80 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
01201f90 35 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |5attrvalue1.....|
01201fa0 61 74 74 72 6e 61 6d 65 36 61 74 74 72 76 61 6c |attrname6attrval|
01201fb0 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
01201fc0 37 61 74 74 72 76 61 6c 75 65 31 00 01 09 0c 00 |7attrvalue1.....|
01201fd0 61 74 74 72 6e 61 6d 65 38 61 74 74 72 76 61 6c |attrname8attrval|
01201fe0 75 65 31 31 31 00 00 00 07 00 00 00 07 00 00 00 |ue111...........|
After lookup_all_xattrs() traveses all inline+xnid entries, the
base_addr becomes the starting pointer of the last xattr entry.
if (last_addr)
cur_addr = XATTR_HDR(last_addr) - 1;
else
cur_addr = txattr_addr;
For example,
before OOB error occurs, the lookup_all_xattrs() calls __find_xattr()
with base_addr = ffffffc05fe98228, max_size = 4 (= XATTR_PADDING_SIZE).
The entry size is 24 bytes = 0x18.
The expected design, the next entry of base_addr (ffffffc05fe98228 + 0x18),
ffffffc05fe98240, should be 4 bytes of zeros.
Because there are faked contents, the stop condition will not be
triggered and an OOB error happenes.
My aim is to block the lookup process by introducing boundary
check.
In this case, we should not access ffffffc05fe98244.
That's why I set the last address to ffffffc05fe98243.
> >>
> >> list_for_each_xattr(entry, base_addr) {
> >> + if ((void *)entry + sizeof(__u32) > max_addr)
> >> + return NULL;
> >> if (entry->e_name_index != index)
> >> continue;
> >> if (entry->e_name_len != len)
> >> @@ -337,9 +342,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
> >> else
> >> cur_addr = txattr_addr;
> >>
> >> - *xe = __find_xattr(cur_addr, index, len, name);
> >> + *xe = __find_xattr(cur_addr, XATTR_PADDING_SIZE, index, len, name);
> >
> > max_size = *base_size - (txattr_addr - cur_addr);
>
> max_size = *base_size - (cur_addr - txattr_addr);
>
> > *xe = __find_xattr(cur_addr, max_size, index, len, name);
> >
> >> check:
> >> - if (IS_XATTR_LAST_ENTRY(*xe)) {
> >> + if (!*xe || IS_XATTR_LAST_ENTRY(*xe)) {
> >
> > If xattr entry across boundary of max xattr space size, maybe we'd better return -EFAULT
> > which can be distinguished from a real -ENODATA error, latter, we can set SBI_NEED_FSCK
> > to give a repairing hint to fsck. :)
> >
I will send v2 patch for review.
> >> err = -ENODATA;
> >> goto out;
> >> }
> >> @@ -606,9 +611,14 @@ static int __f2fs_setxattr(struct inode *inode, int index,
> >> return error;
> >>
> >> /* find entry with wanted name. */
> >> - here = __find_xattr(base_addr, index, len, name);
> >> + here = __find_xattr(base_addr, inline_xattr_size(inode) +
> >> + VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE,
> >> + index, len, name);
> >
> > unsigned int size = F2FS_I(inode)->i_xattr_nid ? VALID_XATTR_BLOCK_SIZE : 0;
> > unsigned int max_size = inline_xattr_size(inode) + size + XATTR_PADDING_SIZE;
> >
> > here = __find_xattr(..., max_size, ...);
> >
> > if (!here)
> > return -EFAULT;
> >
> > Thanks,
> >
> >>
> >> - found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
> >> + if (!here)
> >> + found = 0;
> >> + else
> >> + found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
> >>
> >> if (found) {
> >> if ((flags & XATTR_CREATE)) {
> >>
> >
> >
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
Hi Randall,
On 2019/4/9 16:36, Randall Huang wrote:
> On Mon, Apr 08, 2019 at 08:14:43PM +0800, Chao Yu wrote:
>> On 2019/4/8 20:03, Chao Yu wrote:
>>> Hi Randall,
>>>
>>> On 2019/4/8 16:50, Randall Huang wrote:
>>>> When we traverse xattr entries via __find_xattr(),
>>>> if the raw filesystem content is faked or any hardware failure occurs,
>>>> out-of-bound error can be detected by KASAN.
>>>> Fix the issue by introducing boundary check.
>>>>
>>>> [ 38.402878] c7 1827 BUG: KASAN: slab-out-of-bounds in f2fs_getxattr+0x518/0x68c
>>>> [ 38.402891] c7 1827 Read of size 4 at addr ffffffc0b6fb35dc by task
>>>> [ 38.402935] c7 1827 Call trace:
>>>> [ 38.402952] c7 1827 [<ffffff900809003c>] dump_backtrace+0x0/0x6bc
>>>> [ 38.402966] c7 1827 [<ffffff9008090030>] show_stack+0x20/0x2c
>>>> [ 38.402981] c7 1827 [<ffffff900871ab10>] dump_stack+0xfc/0x140
>>>> [ 38.402995] c7 1827 [<ffffff9008325c40>] print_address_description+0x80/0x2d8
>>>> [ 38.403009] c7 1827 [<ffffff900832629c>] kasan_report_error+0x198/0x1fc
>>>> [ 38.403022] c7 1827 [<ffffff9008326104>] kasan_report_error+0x0/0x1fc
>>>> [ 38.403037] c7 1827 [<ffffff9008325000>] __asan_load4+0x1b0/0x1b8
>>>> [ 38.403051] c7 1827 [<ffffff90085fcc44>] f2fs_getxattr+0x518/0x68c
>>>> [ 38.403066] c7 1827 [<ffffff90085fc508>] f2fs_xattr_generic_get+0xb0/0xd0
>>>> [ 38.403080] c7 1827 [<ffffff9008395708>] __vfs_getxattr+0x1f4/0x1fc
>>>> [ 38.403096] c7 1827 [<ffffff9008621bd0>] inode_doinit_with_dentry+0x360/0x938
>>>> [ 38.403109] c7 1827 [<ffffff900862d6cc>] selinux_d_instantiate+0x2c/0x38
>>>> [ 38.403123] c7 1827 [<ffffff900861b018>] security_d_instantiate+0x68/0x98
>>>> [ 38.403136] c7 1827 [<ffffff9008377db8>] d_splice_alias+0x58/0x348
>>>> [ 38.403149] c7 1827 [<ffffff900858d16c>] f2fs_lookup+0x608/0x774
>>>> [ 38.403163] c7 1827 [<ffffff900835eacc>] lookup_slow+0x1e0/0x2cc
>>>> [ 38.403177] c7 1827 [<ffffff9008367fe0>] walk_component+0x160/0x520
>>>> [ 38.403190] c7 1827 [<ffffff9008369ef4>] path_lookupat+0x110/0x2b4
>>>> [ 38.403203] c7 1827 [<ffffff900835dd38>] filename_lookup+0x1d8/0x3a8
>>>> [ 38.403216] c7 1827 [<ffffff900835eeb0>] user_path_at_empty+0x54/0x68
>>>> [ 38.403229] c7 1827 [<ffffff9008395f44>] SyS_getxattr+0xb4/0x18c
>>>> [ 38.403241] c7 1827 [<ffffff9008084200>] el0_svc_naked+0x34/0x38
>>>>
>>>> Bug: 126558260
>>>>
>>>> Signed-off-by: Randall Huang <[email protected]>
>>>> ---
>>>> fs/f2fs/xattr.c | 22 ++++++++++++++++------
>>>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>>> index 848a785abe25..0531c1e38275 100644
>>>> --- a/fs/f2fs/xattr.c
>>>> +++ b/fs/f2fs/xattr.c
>>>> @@ -202,12 +202,17 @@ static inline const struct xattr_handler *f2fs_xattr_handler(int index)
>>>> return handler;
>>>> }
>>>>
>>>> -static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
>>>> - size_t len, const char *name)
>>>> +static struct f2fs_xattr_entry *__find_xattr(void *base_addr,
>>>> + int base_addr_limit, int index,
>>>
>>> unsigned int max_size,
>>>
>>>> + size_t len, const char *name)
>>>> {
>>>> struct f2fs_xattr_entry *entry;
>>>> + void *max_addr = base_addr + ENTRY_SIZE(XATTR_ENTRY(base_addr)) +
>>>> + base_addr_limit - 1;
>>>
>>> If I'm not missing something, shouldn't it be?
>>>
>>> void *max_addr = base_addr + max_size;
>>>
> Hi Chao,
> Let me show the buggy example of xattr entries.
>
> Here is the buggy xattr entries.
> The address 0x1201f24 - 0x1201fcb is correct content.
> (Each entry occupies 4 + 9 + 10 + 1 bytes, 24 bytes)
> Hacker append a faked entry at 0x1201fcc - 0x1201fe5
> (entry length = 4 + 9 + 12 + 1 bytes)
>
> 01201f00 00 00 00 00 00 00 00 00 00 00 00 00 11 20 f5 f2 |............. ..|
> 01201f10 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 01201f20 00 00 00 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |........attrname|
> 01201f30 31 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |1attrvalue1.....|
> 01201f40 61 74 74 72 6e 61 6d 65 32 61 74 74 72 76 61 6c |attrname2attrval|
> 01201f50 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
> 01201f60 33 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |3attrvalue1.....|
> 01201f70 61 74 74 72 6e 61 6d 65 34 61 74 74 72 76 61 6c |attrname4attrval|
> 01201f80 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
> 01201f90 35 61 74 74 72 76 61 6c 75 65 31 00 01 09 0a 00 |5attrvalue1.....|
> 01201fa0 61 74 74 72 6e 61 6d 65 36 61 74 74 72 76 61 6c |attrname6attrval|
> 01201fb0 75 65 31 00 01 09 0a 00 61 74 74 72 6e 61 6d 65 |ue1.....attrname|
> 01201fc0 37 61 74 74 72 76 61 6c 75 65 31 00 01 09 0c 00 |7attrvalue1.....|
> 01201fd0 61 74 74 72 6e 61 6d 65 38 61 74 74 72 76 61 6c |attrname8attrval|
> 01201fe0 75 65 31 31 31 00 00 00 07 00 00 00 07 00 00 00 |ue111...........|
Thanks for the detailed info. :)
>
> After lookup_all_xattrs() traveses all inline+xnid entries, the
> base_addr becomes the starting pointer of the last xattr entry.
>
> if (last_addr)
> cur_addr = XATTR_HDR(last_addr) - 1;
> else
> cur_addr = txattr_addr;
Actually, if last_addr is valid, it means we have traversed inline xattr space,
but haven't found target xattr entry yet, then we will make cur_addr pointing to
((void *)last_addr - sizeof(struct f2fs_xattr_header *)), that's because in
__find_xattr, list_for_each_xattr() will treat @base_addr as the pointer which
points to xattr header.
Thanks,
>
> For example,
> before OOB error occurs, the lookup_all_xattrs() calls __find_xattr()
> with base_addr = ffffffc05fe98228, max_size = 4 (= XATTR_PADDING_SIZE).
>
> The entry size is 24 bytes = 0x18.
> The expected design, the next entry of base_addr (ffffffc05fe98228 + 0x18),
> ffffffc05fe98240, should be 4 bytes of zeros.
>
> Because there are faked contents, the stop condition will not be
> triggered and an OOB error happenes.
>
> My aim is to block the lookup process by introducing boundary
> check.
>
> In this case, we should not access ffffffc05fe98244.
> That's why I set the last address to ffffffc05fe98243.
>
>>>>
>>>> list_for_each_xattr(entry, base_addr) {
>>>> + if ((void *)entry + sizeof(__u32) > max_addr)
>>>> + return NULL;
>>>> if (entry->e_name_index != index)
>>>> continue;
>>>> if (entry->e_name_len != len)
>>>> @@ -337,9 +342,9 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
>>>> else
>>>> cur_addr = txattr_addr;
>>>>
>>>> - *xe = __find_xattr(cur_addr, index, len, name);
>>>> + *xe = __find_xattr(cur_addr, XATTR_PADDING_SIZE, index, len, name);
>>>
>>> max_size = *base_size - (txattr_addr - cur_addr);
>>
>> max_size = *base_size - (cur_addr - txattr_addr);
>>
>>> *xe = __find_xattr(cur_addr, max_size, index, len, name);
>>>
>>>> check:
>>>> - if (IS_XATTR_LAST_ENTRY(*xe)) {
>>>> + if (!*xe || IS_XATTR_LAST_ENTRY(*xe)) {
>>>
>>> If xattr entry across boundary of max xattr space size, maybe we'd better return -EFAULT
>>> which can be distinguished from a real -ENODATA error, latter, we can set SBI_NEED_FSCK
>>> to give a repairing hint to fsck. :)
>>>
> I will send v2 patch for review.
>>>> err = -ENODATA;
>>>> goto out;
>>>> }
>>>> @@ -606,9 +611,14 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>>>> return error;
>>>>
>>>> /* find entry with wanted name. */
>>>> - here = __find_xattr(base_addr, index, len, name);
>>>> + here = __find_xattr(base_addr, inline_xattr_size(inode) +
>>>> + VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE,
>>>> + index, len, name);
>>>
>>> unsigned int size = F2FS_I(inode)->i_xattr_nid ? VALID_XATTR_BLOCK_SIZE : 0;
>>> unsigned int max_size = inline_xattr_size(inode) + size + XATTR_PADDING_SIZE;
>>>
>>> here = __find_xattr(..., max_size, ...);
>>>
>>> if (!here)
>>> return -EFAULT;
>>>
>>> Thanks,
>>>
>>>>
>>>> - found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>>>> + if (!here)
>>>> + found = 0;
>>>> + else
>>>> + found = IS_XATTR_LAST_ENTRY(here) ? 0 : 1;
>>>>
>>>> if (found) {
>>>> if ((flags & XATTR_CREATE)) {
>>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
>