[Linux-kernel-mentees] [PATCH] hfs, hfsplus: Fix NULL pointer dereference in hfs_find_init()

[Peilin Ye]

Prevent hfs_find_init() from dereferencing `tree` as NULL.

Reported-and-tested-by: syzbot+7ca256...@syzkaller.appspotmail.com
Signed-off-by: Peilin Ye <yepei...@gmail.com>
---
fs/hfs/bfind.c | 3 +++
fs/hfsplus/bfind.c | 3 +++
2 files changed, 6 insertions(+)

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index 4af318fbda77..880b7ea2c0fc 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -16,6 +16,9 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
{
void *ptr;

+ if (!tree)
+ return -EINVAL;
+
fd->tree = tree;
fd->bnode = NULL;
ptr = kmalloc(tree->max_key_len * 2 + 4, GFP_KERNEL);
diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
index ca2ba8c9f82e..85bef3e44d7a 100644
--- a/fs/hfsplus/bfind.c
+++ b/fs/hfsplus/bfind.c
@@ -16,6 +16,9 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
{
void *ptr;

+ if (!tree)
+ return -EINVAL;
+
fd->tree = tree;
fd->bnode = NULL;
ptr = kmalloc(tree->max_key_len * 2 + 4, GFP_KERNEL);
--
2.25.1

[Greg Kroah-Hartman]

How can tree ever be NULL in these calls? Shouldn't that be fixed as
the root problem here?

thanks,

greg k-h

[Peilin Ye]

I see, I will try to figure out what is going on with the reproducer.

Thank you,
Peilin Ye

[Greg Kroah-Hartman]

That's good to figure out. Note, your patch might be the correct thing
to do, as that might be an allowed way to call the function. But in
looking at all the callers, they seem to think they have a valid pointer
at the moment, so perhaps if this check is added, some other root
problem is papered over to be only found later on?

thanks,

greg k-h

[Dan Carpenter]

Yeah, the patch doesn't work at all. I looked at one call tree and it
is:

hfs_mdb_get() tries to allocate HFS_SB(sb)->ext_tree.

HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
^^^^^^^^

hfs_btree_open() calls page = read_mapping_page(mapping, 0, NULL);
read_mapping_page() calls mapping->a_ops->readpage() which leads to
hfs_readpage() which leads to hfs_ext_read_extent() which calls
res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd);
^^^^^^^^

So we need ->ext_tree to be non-NULL before we can set ->ext_tree to be
non-NULL... :/

I wonder how long this has been broken and if we should just delete the
AFS file system.

regards,
dan carpenter

[Big Budsupply]

Hello guys hope you are doing good! we are Bigbudsupply we grow and sell the best medical marijuana product, we are looking for long time customers, you can Email us /Bigbud...@gmail.com

Text/+14432672189

Looking forward to working with you guys

--

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/20200812085904.GA16441%40kadam.

[Peilin Ye]

That's right - Yesterday I noticed that this function has a number of
callers who don't check `tree` at all, so I thought maybe we just add
the check here...Turned out to be quite the opposite.

Thank you,
Peilin Ye

[Peilin Ye]

On Wed, Aug 12, 2020 at 11:59:04AM +0300, Dan Carpenter wrote:
> Yeah, the patch doesn't work at all. I looked at one call tree and it
> is:
>
> hfs_mdb_get() tries to allocate HFS_SB(sb)->ext_tree.
>
> HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
> ^^^^^^^^
>
> hfs_btree_open() calls page = read_mapping_page(mapping, 0, NULL);
> read_mapping_page() calls mapping->a_ops->readpage() which leads to
> hfs_readpage() which leads to hfs_ext_read_extent() which calls
> res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd);
> ^^^^^^^^

Thank you for pointing this out! I will try to come up with a better way
to fix it.

Peilin Ye

[Ernesto A. Fernández]

Hi,

On Wed, Aug 12, 2020 at 11:59:04AM +0300, Dan Carpenter wrote:

> Yeah, the patch doesn't work at all. I looked at one call tree and it
> is:
>
> hfs_mdb_get() tries to allocate HFS_SB(sb)->ext_tree.
>
> HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
> ^^^^^^^^
>
> hfs_btree_open() calls page = read_mapping_page(mapping, 0, NULL);
> read_mapping_page() calls mapping->a_ops->readpage() which leads to
> hfs_readpage() which leads to hfs_ext_read_extent() which calls
> res = hfs_find_init(HFS_SB(inode->i_sb)->ext_tree, &fd);
> ^^^^^^^^
>
> So we need ->ext_tree to be non-NULL before we can set ->ext_tree to be
> non-NULL... :/

For HFS+, the first 8 extents for a file are kept inside its own fork data
structure, not in the extent tree. So, in normal operation, you don't need
to search the extent tree to find the first page of the extent tree itself.
The HFS layout is different, but it should work the same way.

Of course this sort of thing can still be triggered by crafted filesystems.
If that's what the reproducer is about, I think just returning an error is
reasonable. But these modules will never be safe against attacks such as
this.

[Ernesto A. Fernández]

On Wed, Aug 12, 2020 at 05:24:20PM -0300, Ernesto A. Fernández wrote:
> If that's what the reproducer is about, I think just returning an error is
> reasonable.

I guess it would be better to put a check inside hfsplus_inode_read_fork(),
to verify that the first extent is always in the right place and wide
enough.