fs/ntfs/attrib.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
Looks like it is improper check order that causes this bug.
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
fs/ntfs/attrib.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..6480cd2d371d 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,10 +594,11 @@ 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);
+ if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+ break;
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)
+ if (name_end > mrec_end)
break;
ctx->attr = a;
if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
--
2.25.1在 2022/8/26 20:27, Hawkins Jiawei 写道:
> syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> Looks like it is improper check order that causes this bug.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> fs/ntfs/attrib.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..6480cd2d371d 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,10 +594,11 @@ 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);
> + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> + break;
> 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)
> + if (name_end > mrec_end)
> break;
> ctx->attr = a;
> if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
>
The reason is that a->length is 0, it will occur uaf when deref any
field of ATTR_RECORD.
It seems that changing check order will not fix root cause, if the
condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false,
uaf will still occur.
Do you have any thoughts on this ?
On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@huawei.com> wrote:
>
> 在 2022/8/26 20:27, Hawkins Jiawei 写道:
> > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > Looks like it is improper check order that causes this bug.
> >
> > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> > ---
> > fs/ntfs/attrib.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -594,10 +594,11 @@ 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);
> > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > + break;
> > 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)
> > + if (name_end > mrec_end)
> > break;
> > ctx->attr = a;
> > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> >
>
> The reason is that a->length is 0, it will occur uaf when deref any
> field of ATTR_RECORD.
>
> It seems that changing check order will not fix root cause, if the
> condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false,
> uaf will still occur.
>
> Do you have any thoughts on this ?
Hi, chenxiaosong
I think changing the check order is able to fix this bug. But we may need
to check whether mft record header is out of bounds, maybe
"if ((u8*)a < (u8*)ctx->mrec || (u8*)a + sizeof(MFT_RECORD) > mrec_end)"
Because we just need to check if this ATTR_RECORD is in valid addresss. As for
situation that a->length is 0, there seems already a check in the loop
(Correct me if I am wrong):
> static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> const u32 name_len, const IGNORE_CASE_BOOL ic,
> const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
> {
> ...
>
> 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)
> break;
> ctx->attr = a;
> if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> a->type == AT_END))
> return -ENOENT;
> if (unlikely(!a->length))
> break;
> ...
> }
And as for the root cause for use-after-free read, I think it is the
ctx->attr->length to be blamed.
To be more specific, when kernel loads the struct MFT_RECORD from disk
in ntfs_read_inode_mount(), m's attrs_offset field should less than
m's bytes_allocated field, or it may out of the bounds:
int ntfs_read_inode_mount(struct inode *vi)
{
...
MFT_RECORD *m = NULL;
i = vol->mft_record_size;
...
m = (MFT_RECORD*)ntfs_malloc_nofs(i);
/* Determine the first block of the $MFT/$DATA attribute. */
block = vol->mft_lcn << vol->cluster_size_bits >>
sb->s_blocksize_bits;
nr_blocks = vol->mft_record_size >> sb->s_blocksize_bits;
...
/* Load $MFT/$DATA's first mft record. */
for (i = 0; i < nr_blocks; i++) {
bh = sb_bread(sb, block++);
if (!bh) {
ntfs_error(sb, "Device read failed.");
goto err_out;
}
memcpy((char*)m + (i << sb->s_blocksize_bits), bh->b_data,
sb->s_blocksize);
brelse(bh);
}
if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
goto err_out;
}
...
ctx = ntfs_attr_get_search_ctx(ni, m);
if (!ctx) {
err = -ENOMEM;
goto err_out;
}
/* Find the attribute list attribute if present. */
err = ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, 0, 0, NULL, 0, ctx);
...
}
> ==================================================================
> 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>
> 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
> ==================================================================
So check these fields as follow should fix the root cause for this
use-after-free bug(Correct me if I am wrong). I test this patch locally,
it doesn't trigger any issue.
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index db0f1995aedd..6ba99e109ca9 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1822,6 +1822,11 @@ int ntfs_read_inode_mount(struct inode *vi)
goto err_out;
}
+ if (m->attrs_offset >= le32_to_cpu(m->bytes_allocated)) {
+ ntfs_error(sb, "Incorrect mft record attrs_offset %u", m->attrs_offset);
+ goto err_out;
+ }
+
/* Apply the mst fixups. */
if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
/* FIXME: Try to use the $MFTMirr now. */
What's your opinion?
On Sat, 27 Aug 2022 at 15:59, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On Sat, 27 Aug 2022 at 09:29, chenxiaosong (A) <chenxiaosong2@huawei.com> wrote:
> >
> > 在 2022/8/26 20:27, Hawkins Jiawei 写道:
> > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > Looks like it is improper check order that causes this bug.
> > >
> > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> > > ---
> > > fs/ntfs/attrib.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -594,10 +594,11 @@ 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);
> > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > + break;
> > > 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)
> > > + if (name_end > mrec_end)
> > > break;
> > > ctx->attr = a;
> > > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> > >
> >
> > The reason is that a->length is 0, it will occur uaf when deref any
> > field of ATTR_RECORD.
> >
> > It seems that changing check order will not fix root cause, if the
> > condition "if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)" is false,
> > uaf will still occur.
> >
> > Do you have any thoughts on this ?
> Hi, chenxiaosong
>
> I think changing the check order is able to fix this bug. But we may need
> to check whether mft record header is out of bounds, maybe
> "if ((u8*)a < (u8*)ctx->mrec || (u8*)a + sizeof(MFT_RECORD) > mrec_end)"
>
> Because we just need to check if this ATTR_RECORD is in valid addresss. As for
> situation that a->length is 0, there seems already a check in the loop
> (Correct me if I am wrong):
> > static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > const u32 name_len, const IGNORE_CASE_BOOL ic,
> > const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
> > {
> > ...
> >
> > 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)
> > break;
> > ctx->attr = a;
> > if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> > a->type == AT_END))
> > return -ENOENT;
> > if (unlikely(!a->length))
> > break;
> > ...
> > }
>
> And as for the root cause for use-after-free read, I think it is the
> ctx->attr->length to be blamed.
Apologize for my typing error, it is the ctx->attr should be blamed:
static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
const u32 name_len, const IGNORE_CASE_BOOL ic,
const u8 *val, const u32 val_len, ntfs_attr_search_ctx *ctx)
{
ATTR_RECORD *a;
ntfs_volume *vol = ctx->ntfs_ino->vol;
ntfschar *upcase = vol->upcase;
u32 upcase_len = vol->upcase_len;
/*
* Iterate over attributes in mft record starting at @ctx->attr, or the
* attribute following that, if @ctx->is_first is 'true'.
*/
if (ctx->is_first) {
a = ctx->attr;
ctx->is_first = false;
} else
a = (ATTR_RECORD*)((u8*)ctx->attr +
le32_to_cpu(ctx->attr->length));
...
}
The "ctx->attr" is not invalid, then the pointer "a" will probably also
points to an invalid memory, which triggers a use-after-free read.
This also probably explains why the memory state around the buggy address are
all invalid in KASAN, instead of at the bounder between valid memory
and invalid memory
Yet the analysis below is still correct. ctx->attr is assigned with
"((u8*)mrec + le16_to_cpu(mrec->attrs_offset))" in
ntfs_attr_init_search_ctx()
>
> To be more specific, when kernel loads the struct MFT_RECORD from disk
> in ntfs_read_inode_mount(), m's attrs_offset field should less than
> m's bytes_allocated field, or it may out of the bounds:
>
> int ntfs_read_inode_mount(struct inode *vi)
> {
> ...
> MFT_RECORD *m = NULL;
> i = vol->mft_record_size;
>
> ...
> m = (MFT_RECORD*)ntfs_malloc_nofs(i);
>
> /* Determine the first block of the $MFT/$DATA attribute. */
> block = vol->mft_lcn << vol->cluster_size_bits >>
> sb->s_blocksize_bits;
> nr_blocks = vol->mft_record_size >> sb->s_blocksize_bits;
>
> ...
>
> /* Load $MFT/$DATA's first mft record. */
> for (i = 0; i < nr_blocks; i++) {
> bh = sb_bread(sb, block++);
> if (!bh) {
> ntfs_error(sb, "Device read failed.");
> goto err_out;
> }
> memcpy((char*)m + (i << sb->s_blocksize_bits), bh->b_data,
> sb->s_blocksize);
> brelse(bh);
> }
>
> if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
> ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
> le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
> goto err_out;
> }
>
> ...
>
> ctx = ntfs_attr_get_search_ctx(ni, m);
> if (!ctx) {
> err = -ENOMEM;
> goto err_out;
> }
>
> /* Find the attribute list attribute if present. */
> err = ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, 0, 0, NULL, 0, ctx);
>
> ...
> }
>
> > ==================================================================
> > 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>
> > 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
> > ==================================================================
> So check these fields as follow should fix the root cause for this
> use-after-free bug(Correct me if I am wrong). I test this patch locally,
> it doesn't trigger any issue.
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index db0f1995aedd..6ba99e109ca9 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1822,6 +1822,11 @@ int ntfs_read_inode_mount(struct inode *vi)
> goto err_out;
> }
>
> + if (m->attrs_offset >= le32_to_cpu(m->bytes_allocated)) {
> + ntfs_error(sb, "Incorrect mft record attrs_offset %u", m->attrs_offset);
> + goto err_out;
> + }
> +
> /* Apply the mst fixups. */
> if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
> /* FIXME: Try to use the $MFTMirr now. */
>
>
> What's your opinion?
On Sat, Aug 27, 2022 at 10:49:44PM +0800, Hawkins Jiawei wrote: > > > > And as for the root cause for use-after-free read, I think it is the > > ctx->attr->length to be blamed. > Apologize for my typing error, it is the ctx->attr should be blamed: > Yep. Both patches are required. regards, dan carpenter
> syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> Looks like it is improper check order that causes this bug.
Sorry for wrong command.
#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..6480cd2d371d 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,10 +594,11 @@ 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);
+ if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+ break;
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)
+ if (name_end > mrec_end)
break;
ctx->attr = a;
if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
--
2.25.1On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote: > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > Looks like it is improper check order that causes this bug. um, what bug? > Sorry for wrong command. > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master Please prepare a full changelog for the next version? Describe the user-visible runtime effects of the bug, describe what the code does wrong and how the patch repairs it.
On Sat, 27 Aug 2022 at 10:42, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote: > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > > > Looks like it is improper check order that causes this bug. > > um, what bug? > > > Sorry for wrong command. > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > Please prepare a full changelog for the next version? Describe the > user-visible runtime effects of the bug, describe what the code does > wrong and how the patch repairs it. > I am sorry for that improper email. I send that email just wants to confirm whether my guess is right by syzbot. That is not an official patch email. Thanks for your remind, I will take care next time!
On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > Looks like it is improper check order that causes this bug.
>
> Sorry for wrong command.
> #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..6480cd2d371d 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,10 +594,11 @@ 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);
> + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> + break;
This definitely seems like a bug. But your code won't build. Syzbot
must have -Werror turned off?
Btw, this was in the original code, but those casts are ugly. Ideally
there would be some way to get rid of them. But otherwise at least
put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> 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)
> + if (name_end > mrec_end)
> break;
regards,
dan carpenter
On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > Looks like it is improper check order that causes this bug.
> >
> > Sorry for wrong command.
> > #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..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -594,10 +594,11 @@ 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);
> > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > + break;
>
> This definitely seems like a bug. But your code won't build. Syzbot
> must have -Werror turned off?
Hi Dan,
Did you mean we should put the variable declares at the beginning of the function?
(Correct me if I understand anything wrong)
>
> Btw, this was in the original code, but those casts are ugly. Ideally
> there would be some way to get rid of them. But otherwise at least
> put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
>
> > 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)
> > + if (name_end > mrec_end)
> > break;
>
> regards,
> dan carpenter
So maybe I can try to refactor these codes. But I wonder if this can be
done in a seperate bug
On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > >
> > > > Looks like it is improper check order that causes this bug.
> > >
> > > Sorry for wrong command.
> > > #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..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -594,10 +594,11 @@ 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);
> > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > + break;
> >
> > This definitely seems like a bug. But your code won't build. Syzbot
> > must have -Werror turned off?
> Hi Dan,
> Did you mean we should put the variable declares at the beginning of the function?
> (Correct me if I understand anything wrong)
You can declare it at the beginning of the block.
>
> >
> > Btw, this was in the original code, but those casts are ugly. Ideally
> > there would be some way to get rid of them. But otherwise at least
> > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> >
> > > 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)
> > > + if (name_end > mrec_end)
> > > break;
> >
> > regards,
> > dan carpenter
> So maybe I can try to refactor these codes. But I wonder if this can be
> done in a seperate bug
The kernel has a strict "one thing per patch rule". Those rules are
for reviewers and easier backporting. So the trick is to write the
commit message to persuade the reviewer that the way you've written the
patch is the easiest way to review it. So here is how I would write the
commit message:
[PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
This code deferences "a" to calculate "name_end" and then it checks to
ensure that "a" is within bounds. Move the bounds checks earlier and
add some comments to make it more clear what they're doing. Then
calculate "name_end" and check that.
(Btw, are the wrap around checks really sufficient? It seems like it
could wrap to something still within the ->mrec buffer but before the
current entry so it would end up in a forever loop or something?)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..90d567acb2a3 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,11 +594,20 @@ 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 for overflow */
+ if ((u8 *)a > mrec_end)
break;
+
+ 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))
On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > >
> > > > > Looks like it is improper check order that causes this bug.
> > > >
> > > > Sorry for wrong command.
> > > > #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..6480cd2d371d 100644
> > > > --- a/fs/ntfs/attrib.c
> > > > +++ b/fs/ntfs/attrib.c
> > > > @@ -594,10 +594,11 @@ 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);
> > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > + break;
> > >
> > > This definitely seems like a bug. But your code won't build. Syzbot
> > > must have -Werror turned off?
> > Hi Dan,
> > Did you mean we should put the variable declares at the beginning of the function?
> > (Correct me if I understand anything wrong)
>
> You can declare it at the beginning of the block.
OK, I will do like that.
>
> >
> > >
> > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > there would be some way to get rid of them. But otherwise at least
> > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > >
> > > > 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)
> > > > + if (name_end > mrec_end)
> > > > break;
> > >
> > > regards,
> > > dan carpenter
> > So maybe I can try to refactor these codes. But I wonder if this can be
> > done in a seperate bug
>
> The kernel has a strict "one thing per patch rule". Those rules are
> for reviewers and easier backporting. So the trick is to write the
> commit message to persuade the reviewer that the way you've written the
> patch is the easiest way to review it. So here is how I would write the
> commit message:
>
> [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
>
> This code deferences "a" to calculate "name_end" and then it checks to
> ensure that "a" is within bounds. Move the bounds checks earlier and
> add some comments to make it more clear what they're doing. Then
> calculate "name_end" and check that.
>
> (Btw, are the wrap around checks really sufficient? It seems like it
> could wrap to something still within the ->mrec buffer but before the
> current entry so it would end up in a forever loop or something?)
I am not for sure, but it seems that it is OK before.
As for the forever loop, there is a break when a->length is 0 in the loop,
So I think it probably would not end up in a forever loop?(Correct me if
I am wrong)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..90d567acb2a3 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,11 +594,20 @@ 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 for overflow */
> + if ((u8 *)a > mrec_end)
> break;
> +
> + 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))
Thanks for your suggestion, I will refactor these codes in this way.
On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > >
> > > > > > Looks like it is improper check order that causes this bug.
> > > > >
> > > > > Sorry for wrong command.
> > > > > #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..6480cd2d371d 100644
> > > > > --- a/fs/ntfs/attrib.c
> > > > > +++ b/fs/ntfs/attrib.c
> > > > > @@ -594,10 +594,11 @@ 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);
> > > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > > + break;
> > > >
> > > > This definitely seems like a bug. But your code won't build. Syzbot
> > > > must have -Werror turned off?
> > > Hi Dan,
> > > Did you mean we should put the variable declares at the beginning of the function?
> > > (Correct me if I understand anything wrong)
> >
> > You can declare it at the beginning of the block.
> OK, I will do like that.
>
> >
> > >
> > > >
> > > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > > there would be some way to get rid of them. But otherwise at least
> > > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > > >
> > > > > 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)
> > > > > + if (name_end > mrec_end)
> > > > > break;
> > > >
> > > > regards,
> > > > dan carpenter
> > > So maybe I can try to refactor these codes. But I wonder if this can be
> > > done in a seperate bug
> >
> > The kernel has a strict "one thing per patch rule". Those rules are
> > for reviewers and easier backporting. So the trick is to write the
> > commit message to persuade the reviewer that the way you've written the
> > patch is the easiest way to review it. So here is how I would write the
> > commit message:
> >
> > [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
> >
> > This code deferences "a" to calculate "name_end" and then it checks to
> > ensure that "a" is within bounds. Move the bounds checks earlier and
> > add some comments to make it more clear what they're doing. Then
> > calculate "name_end" and check that.
> >
> > (Btw, are the wrap around checks really sufficient? It seems like it
> > could wrap to something still within the ->mrec buffer but before the
> > current entry so it would end up in a forever loop or something?)
> I am not for sure, but it seems that it is OK before.
> As for the forever loop, there is a break when a->length is 0 in the loop,
> So I think it probably would not end up in a forever loop?(Correct me if
> I am wrong)
>
Checking for zero is not sufficient because it can wrap on 32 bit
systems. It needs something like:
return -ENOENT;
len = le32_to_cpu(a->length);
if (!len ||
(void *)a + len < (void *)a ||
(void *)a + len > mrec_end)
break;
if (a->type != type)
continue;
Sort of ugly, but hopefully it gives the idea of what I'm saying.
regards,
dan carpenter
On Sat, 27 Aug 2022 at 18:59, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> > On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > > >
> > > > > > > Looks like it is improper check order that causes this bug.
> > > > > >
> > > > > > Sorry for wrong command.
> > > > > > #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..6480cd2d371d 100644
> > > > > > --- a/fs/ntfs/attrib.c
> > > > > > +++ b/fs/ntfs/attrib.c
> > > > > > @@ -594,10 +594,11 @@ 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);
> > > > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > > > + break;
> > > > >
> > > > > This definitely seems like a bug. But your code won't build. Syzbot
> > > > > must have -Werror turned off?
> > > > Hi Dan,
> > > > Did you mean we should put the variable declares at the beginning of the function?
> > > > (Correct me if I understand anything wrong)
> > >
> > > You can declare it at the beginning of the block.
> > OK, I will do like that.
> >
> > >
> > > >
> > > > >
> > > > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > > > there would be some way to get rid of them. But otherwise at least
> > > > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > > > >
> > > > > > 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)
> > > > > > + if (name_end > mrec_end)
> > > > > > break;
> > > > >
> > > > > regards,
> > > > > dan carpenter
> > > > So maybe I can try to refactor these codes. But I wonder if this can be
> > > > done in a seperate bug
> > >
> > > The kernel has a strict "one thing per patch rule". Those rules are
> > > for reviewers and easier backporting. So the trick is to write the
> > > commit message to persuade the reviewer that the way you've written the
> > > patch is the easiest way to review it. So here is how I would write the
> > > commit message:
> > >
> > > [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
> > >
> > > This code deferences "a" to calculate "name_end" and then it checks to
> > > ensure that "a" is within bounds. Move the bounds checks earlier and
> > > add some comments to make it more clear what they're doing. Then
> > > calculate "name_end" and check that.
> > >
> > > (Btw, are the wrap around checks really sufficient? It seems like it
> > > could wrap to something still within the ->mrec buffer but before the
> > > current entry so it would end up in a forever loop or something?)
> > I am not for sure, but it seems that it is OK before.
> > As for the forever loop, there is a break when a->length is 0 in the loop,
> > So I think it probably would not end up in a forever loop?(Correct me if
> > I am wrong)
> >
>
> Checking for zero is not sufficient because it can wrap on 32 bit
> systems. It needs something like:
>
> return -ENOENT;
> len = le32_to_cpu(a->length);
> if (!len ||
> (void *)a + len < (void *)a ||
> (void *)a + len > mrec_end)
> break;
> if (a->type != type)
> continue;
>
> Sort of ugly, but hopefully it gives the idea of what I'm saying.
>
> regards,
> dan carpenter
Hi, Dan
Do you mean there may be an overflow on 32bit systems?
It seems that it is, so it may end up in a forever loop
as you point out before.
I will try to add an overflow checking helper to fix it.
On Fri, 26 Aug 2022 at 23:49, Hawkins Jiawei <yin31149@gmail.com> wrote: > > > > > Btw, this was in the original code, but those casts are ugly. Ideally > > there would be some way to get rid of them. But otherwise at least > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec". > > > > > 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) > > > + if (name_end > mrec_end) > > > break; > > > > regards, > > dan carpenter > So maybe I can try to refactor these codes. But I wonder if this can be > done in a seperate bug Sorry for the typing error. I mean refactoring these code in another seperate patch seems better.
Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com Tested on: commit: 4c612826 Merge tag 'net-6.0-rc3' of git://git.kernel.o.. 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=115a8b13080000 kernel config: https://syzkaller.appspot.com/x/.config?x=911efaff115942bb 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=1229de4d080000 Note: testing is done by a robot and is best-effort only.
© 2016 - 2026 Red Hat, Inc.