| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190409083605.GA178602@google.com>
Date: Tue, 9 Apr 2019 16:36:05 +0800
From: Randall Huang <huangrandall@...gle.com>
To: Chao Yu <yuchao0@...wei.com>, jaegeuk@...nel.org,
linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Cc: huangrandall@...gle.com
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to avoid accessing xattr across the
boundary
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@...gle.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@...ts.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
Powered by blists - more mailing lists