Test for KASAN: use-after-free Read in ntfs_attr_find

[Hawkins Jiawei]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+5f8dca...@syzkaller.appspotmail.com

Tested on:

commit: b90cb105 Linux 6.0-rc3
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1449507d080000
kernel config: https://syzkaller.appspot.com/x/.config?x=892a57667b7af6cf
dashboard link: https://syzkaller.appspot.com/bug?extid=5f8dcabe4a3b2c51c607
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=150d306d080000

Note: testing is done by a robot and is best-effort only.

[Hawkins Jiawei]

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+5f8dca...@syzkaller.appspotmail.com

Tested on:

commit: b90cb105 Linux 6.0-rc3
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=160bf9db080000

kernel config: https://syzkaller.appspot.com/x/.config?x=892a57667b7af6cf
dashboard link: https://syzkaller.appspot.com/bug?extid=5f8dcabe4a3b2c51c607
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=129a928b080000

[Dan Carpenter]

On Mon, Aug 29, 2022 at 10:37:37PM +0800, Hawkins Jiawei wrote:
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
>

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAKrof1OjDMLirKbfsABqEtsDj9yW%3DKJQ099HVN9%2B%3D5O%2BfXRtxg%40mail.gmail.com.

> From 2abb85a1cc65d40d951ae34d881b5ba47b737c4f Mon Sep 17 00:00:00 2001
> From: Hawkins Jiawei <yin3...@gmail.com>
> Date: Mon, 29 Aug 2022 20:39:42 +0800
> Subject: [PATCH] ntfs: fix out-of-bounds read in ntfs_attr_find()
>
> Kernel will iterates over ATTR_RECORDs in mft record in ntfs_attr_find().
> To ensure access on these ATTR_RECORDs are within bounds, kernel will
> do some checking during iteration.
>
> The problem is that during checking whether ATTR_RECORD's name is within
> bounds, kernel will dereferences the ATTR_RECORD name_offset field,
> before checking this ATTR_RECORD strcture is within bounds. This problem
> may result out-of-bounds read in ntfs_attr_find(), reported by
> Syzkaller:
>
> ==================================================================
> BUG: KASAN: use-after-free in ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> Read of size 2 at addr ffff88807e352009 by task syz-executor153/3607
>
> [...]
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> print_address_description mm/kasan/report.c:317 [inline]
> print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
> kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
> ntfs_attr_find+0xc02/0xce0 fs/ntfs/attrib.c:597
> ntfs_attr_lookup+0x1056/0x2070 fs/ntfs/attrib.c:1193
> ntfs_read_inode_mount+0x89a/0x2580 fs/ntfs/inode.c:1845
> ntfs_fill_super+0x1799/0x9320 fs/ntfs/super.c:2854
> mount_bdev+0x34d/0x410 fs/super.c:1400
> legacy_get_tree+0x105/0x220 fs/fs_context.c:610
> vfs_get_tree+0x89/0x2f0 fs/super.c:1530
> do_new_mount fs/namespace.c:3040 [inline]
> path_mount+0x1326/0x1e20 fs/namespace.c:3370
> do_mount fs/namespace.c:3383 [inline]
> __do_sys_mount fs/namespace.c:3591 [inline]
> __se_sys_mount fs/namespace.c:3568 [inline]
> __x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [...]
> </TASK>
>
> The buggy address belongs to the physical page:
> page:ffffea0001f8d400 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x7e350
> head:ffffea0001f8d400 order:3 compound_mapcount:0 compound_pincount:0
> flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
> raw: 00fff00000010200 0000000000000000 dead000000000122 ffff888011842140
> raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> Memory state around the buggy address:
> ffff88807e351f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff88807e351f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff88807e352000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff88807e352080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff88807e352100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
> This patch solves it by moving the ATTR_RECORD strcture's bounds
> checking earlier, then checking whether ATTR_RECORD's name
> is within bounds. What's more, this patch also add some comments
> to improve its maintainability.
>
> Signed-off-by: Hawkins Jiawei <yin3...@gmail.com>
> ---
> fs/ntfs/attrib.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..a85e36c02577 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,10 +594,17 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> u8 *mrec_end = (u8 *)ctx->mrec +
> le32_to_cpu(ctx->mrec->bytes_allocated);
> - u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> - a->name_length * sizeof(ntfschar);
> - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> - name_end > mrec_end)
> + u8 *name_end;
> + /* check for wrap around */
> + if ((u8 *)a < (u8 *)ctx->mrec)
> + break;
> + /* check whether ATTR_RECORD a is out-of-bounds */
> + if ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end)
^^^^^^^^^^^^^^^^^^^^^^^
This addition can wrap.

regards,
dan carpenter

[Hawkins Jiawei]

Hi Dan,
You are right.

In fact, I have the third patch to patch the overflow as you suggested
before, I wonder if we can check this overflow together in the third
patch:

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 9b73eec8bcaa..c08798c3519f 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -515,6 +515,22 @@ runlist_element *ntfs_attr_find_vcn_nolock(ntfs_inode *ni, const VCN vcn,
return ERR_PTR(err);
}

+/**
+ * ntfs_check_add_pointer_overflow - Add an unsigned val to a pointer,
+ * and check overflow. If there is an overflow, return True, Otherwise
+ * return False
+ *
+ * @pointer: Pointer to be added
+ * @val: The unsigned val addend
+ */
+static inline bool __must_check ntfs_check_add_pointer_overflow(
+ const void *pointer, uintptr_t val)
+{
+ if (pointer + val < pointer)
+ return true;
+ return false;
+}
+
/**
* ntfs_attr_find - find (next) attribute in mft record
* @type: attribute type to find
@@ -588,23 +604,48 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
if (ctx->is_first) {
a = ctx->attr;
ctx->is_first = false;
- } else
+ } else {
+ if (ntfs_check_add_pointer_overflow(ctx->attr,
+ le32_to_cpu(ctx->attr->length))) {
+ ntfs_error(vol->sb, "Inode is corrupt. Run chkdsk.");
+ NVolSetErrors(vol);
+ return -EIO;
+ }
a = (ATTR_RECORD*)((u8*)ctx->attr +
le32_to_cpu(ctx->attr->length));
- for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
- u8 *mrec_end = (u8 *)ctx->mrec +
- le32_to_cpu(ctx->mrec->bytes_allocated);
- u8 *name_end;
+ }
+
+ for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
+ u8 *mrec_end, *name_end;

/* check for wrap around */

if ((u8 *)a < (u8 *)ctx->mrec)

break;
+
+ /* check for add overflow */
+ if (ntfs_check_add_pointer_overflow(ctx->mrec,
+ le32_to_cpu(ctx->mrec->bytes_allocated)))
+ break;
+ mrec_end = (u8 *)ctx->mrec +
+ le32_to_cpu(ctx->mrec->bytes_allocated);

+
/* check whether ATTR_RECORD a is out-of-bounds */

- if ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end)
+ if (ntfs_check_add_pointer_overflow(a, sizeof(ATTR_RECORD)) ||
+ ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end))
+ break;
+
+ /* check for add overflow */
+ if (ntfs_check_add_pointer_overflow(a, le32_to_cpu(a->length)))
break;

+ /* check for add overflow */
+ if (ntfs_check_add_pointer_overflow(a,
+ (u32)le16_to_cpu(a->name_offset) +
+ a->name_length * sizeof(ntfschar)))
+ break;

name_end = (u8 *)a + le16_to_cpu(a->name_offset) +

a->name_length * sizeof(ntfschar);
+
/* check whether ATTR_RECORD's name is out-of-bounds */
if (name_end > mrec_end)
break;

I add the checking at possbile overflow bug which may leads the forever
loop or bypassing the other check.

(By the way, because this is not an official patch email,
I did not add 'Suggested-by' or 'Reported-and-tested-by'
tags. I will add these tags in my official patch!)

[Hawkins Jiawei]

I refactor this patch, add the new helper function ntfs_safe_add_pointer()
to check and also save the result to the pointer, to improve
its maintainability as below:

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 9b73eec8bcaa..87ae3c7de7f3 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -515,6 +515,43 @@ runlist_element *ntfs_attr_find_vcn_nolock(ntfs_inode *ni, const VCN vcn,

return ERR_PTR(err);
}

+/**
+ * ntfs_check_add_pointer_overflow - Add an unsigned val to a pointer,
+ * and check overflow. If there is an overflow, return True, Otherwise
+ * return False
+ *
+ * @pointer: Pointer to be added
+ * @val: The unsigned val addend
+ */
+static inline bool __must_check ntfs_check_add_pointer_overflow(
+ const void *pointer, uintptr_t val)
+{

+ if (unlikely(pointer + val < pointer))

+ return true;
+ return false;
+}
+

+/**
+ * ntfs_safe_add_pointer - Add an unsigned val to a pointer,
+ * check overflow and store the result. If there is an overflow,
+ * function does not store the result and return False, Otherwise
+ * return True and save the result

+ *
+ * @pointer: Pointer to be added
+ * @val: The unsigned val addend

+ * @dst: Pointer to where to store the result
+ */
+static inline bool __must_check ntfs_safe_add_pointer(
+ void *pointer, uintptr_t val, void *dst)
+{
+ bool overflow = ntfs_check_add_pointer_overflow(pointer, val);
+
+ if (likely(!overflow))
+ *(void **)dst = pointer + val;
+
+ return !overflow;

+}
+
/**
* ntfs_attr_find - find (next) attribute in mft record
* @type: attribute type to find

@@ -588,23 +625,38 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,

if (ctx->is_first) {
a = ctx->attr;
ctx->is_first = false;
- } else

- a = (ATTR_RECORD*)((u8*)ctx->attr +
- le32_to_cpu(ctx->attr->length));

- for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
- u8 *mrec_end = (u8 *)ctx->mrec +
- le32_to_cpu(ctx->mrec->bytes_allocated);
- u8 *name_end;

+ } else if (!ntfs_safe_add_pointer(ctx->attr,
+ le32_to_cpu(ctx->attr->length), &a)) {

+ ntfs_error(vol->sb, "Inode is corrupt. Run chkdsk.");
+ NVolSetErrors(vol);
+ return -EIO;
+ }

+
+ for (;; a = (ATTR_RECORD *)((u8 *)a + le32_to_cpu(a->length))) {

+ u8 *mrec_end, *name_end;

/* check for wrap around */
if ((u8 *)a < (u8 *)ctx->mrec)
break;
+

+ if (!ntfs_safe_add_pointer(ctx->mrec,
+ le32_to_cpu(ctx->mrec->bytes_allocated),
+ &mrec_end))

+ break;
+
/* check whether ATTR_RECORD a is out-of-bounds */

- if ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end)
+ if (ntfs_check_add_pointer_overflow(a, sizeof(ATTR_RECORD)) ||
+ ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end))

break;

- name_end = (u8 *)a + le16_to_cpu(a->name_offset) +

- a->name_length * sizeof(ntfschar);

+ /* check for add overflow */
+ if (ntfs_check_add_pointer_overflow(a, le32_to_cpu(a->length)))

+ break;
+
+ if (!ntfs_safe_add_pointer(a, (u32)le16_to_cpu(a->name_offset) +
+ a->name_length * sizeof(ntfschar),
+ &name_end))
+ break;

/* check whether ATTR_RECORD's name is out-of-bounds */
if (name_end > mrec_end)
break;

[Dan Carpenter]

We already have size_add() and a bunch of other overflow helpers in
include/linux/overflow.h. If we're going to add another then that's
probably the place it should go...

I guess I would prefer a smaller patch instead first. This seems like
a complicated patch to review. We already had two patches ready to
merge and we just needed a third one to check a->length in the middle
of the loop.

>
> > (By the way, because this is not an official patch email,
> > I did not add 'Suggested-by' or 'Reported-and-tested-by'
> > tags. I will add these tags in my official patch!)

No need. You did all the work.

regards,
dan carpenter

[Hawkins Jiawei]

OK, I will only check a->length in the third patch.

Oh, you really help me a lot.
I will add 'Suggested-by' tag in relative patches.

On Mon, 29 Aug 2022 at 23:28, Dan Carpenter <dan.ca...@oracle.com> wrote:
>
> On Mon, Aug 29, 2022 at 10:37:37PM +0800, Hawkins Jiawei wrote:
> >

> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..a85e36c02577 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c

> > @@ -594,10 +594,17 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> >       for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> >               u8 *mrec_end = (u8 *)ctx->mrec +
> >                              le32_to_cpu(ctx->mrec->bytes_allocated);
> > -             u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > -                            a->name_length * sizeof(ntfschar);
> > -             if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > -                 name_end > mrec_end)
> > +             u8 *name_end;
> > +             /* check for wrap around */
> > +             if ((u8 *)a < (u8 *)ctx->mrec)
> > +                     break;
> > +             /* check whether ATTR_RECORD a is out-of-bounds  */
> > +             if ((u8 *)a + sizeof(ATTR_RECORD) > mrec_end)
>                           ^^^^^^^^^^^^^^^^^^^^^^^
> This addition can wrap.
>

> regards,
> dan carpenter
>
As for second patch, I will take care of wrap you point out in previous
email.

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..904734e34507 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,11 +594,23 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,

for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
u8 *mrec_end = (u8 *)ctx->mrec +
le32_to_cpu(ctx->mrec->bytes_allocated);
- u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
- a->name_length * sizeof(ntfschar);
- if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
- name_end > mrec_end)

+ u8 *name_end, *arec_head_end;
+

+ /* check for wrap around */
+ if ((u8 *)a < (u8 *)ctx->mrec)
+ break;
+

+ /* check whether Attribute Record Header is within bounds */
+ arec_head_end = (u8 *)a + sizeof(ATTR_RECORD);
+ if (arec_head_end < (u8 *)a || arec_head_end > mrec_end)
break;
+
+ /* check whether ATTR_RECORD's name is within bounds */
+ name_end = (u8 *)a + le16_to_cpu(a->name_offset) +

+ a->name_length * sizeof(ntfschar);

+ if (name_end > mrec_end)
+ break;
+
ctx->attr = a;
if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
a->type == AT_END))
--
2.25.1

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+5f8dca...@syzkaller.appspotmail.com

Tested on:

commit: dcf8e563 tracing: Define the is_signed_type() macro once

git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

console output: https://syzkaller.appspot.com/x/log.txt?x=14650bdb080000

kernel config: https://syzkaller.appspot.com/x/.config?x=892a57667b7af6cf
dashboard link: https://syzkaller.appspot.com/bug?extid=5f8dcabe4a3b2c51c607
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

patch: https://syzkaller.appspot.com/x/patch.diff?x=139eb49d080000