All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix to avoid accessing xattr across the boundary
@ 2019-04-08  8:50 Randall Huang
  2019-04-08 12:03   ` Chao Yu
  0 siblings, 1 reply; 8+ messages in thread
From: Randall Huang @ 2019-04-08  8:50 UTC (permalink / raw)
  To: jaegeuk, yuchao0, linux-f2fs-devel, linux-kernel; +Cc: huangrandall

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 <huangrandall@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix to avoid accessing xattr across the boundary
  2019-04-08  8:50 [PATCH] f2fs: fix to avoid accessing xattr across the boundary Randall Huang
@ 2019-04-08 12:03   ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-08 12:03 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
> ---
>  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)) {
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix to avoid accessing xattr across the boundary
@ 2019-04-08 12:03   ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-08 12:03 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
> ---
>  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)) {
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary
  2019-04-08 12:03   ` Chao Yu
@ 2019-04-08 12:14     ` Chao Yu
  -1 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-08 12:14 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
>> ---
>>  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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary
@ 2019-04-08 12:14     ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-08 12:14 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
>> ---
>>  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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary
  2019-04-08 12:14     ` Chao Yu
  (?)
@ 2019-04-09  8:36     ` Randall Huang
  2019-04-09 10:10         ` Chao Yu
  -1 siblings, 1 reply; 8+ messages in thread
From: Randall Huang @ 2019-04-09  8:36 UTC (permalink / raw)
  To: Chao Yu, jaegeuk, linux-f2fs-devel, linux-kernel; +Cc: huangrandall

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 <huangrandall@google.com>
> >> ---
> >>  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
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary
  2019-04-09  8:36     ` Randall Huang
@ 2019-04-09 10:10         ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-09 10:10 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
>>>> ---
>>>>  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
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary
@ 2019-04-09 10:10         ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-04-09 10:10 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

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 <huangrandall@google.com>
>>>> ---
>>>>  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
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>> .
>>>
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-09 10:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08  8:50 [PATCH] f2fs: fix to avoid accessing xattr across the boundary Randall Huang
2019-04-08 12:03 ` Chao Yu
2019-04-08 12:03   ` Chao Yu
2019-04-08 12:14   ` [f2fs-dev] " Chao Yu
2019-04-08 12:14     ` Chao Yu
2019-04-09  8:36     ` Randall Huang
2019-04-09 10:10       ` Chao Yu
2019-04-09 10:10         ` Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.