* [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; 5+ 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] 5+ 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 2019-04-08 12:14 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 5+ 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] 5+ 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 2019-04-09 8:36 ` Randall Huang 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the boundary 2019-04-08 12:14 ` [f2fs-dev] " Chao Yu @ 2019-04-09 8:36 ` Randall Huang 2019-04-09 10:10 ` Chao Yu 0 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2019-04-09 10:10 UTC | newest] Thread overview: 5+ 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:14 ` [f2fs-dev] " Chao Yu 2019-04-09 8:36 ` Randall Huang 2019-04-09 10:10 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).