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

[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+7ca256d0da4af073b2e2@syzkaller.appspotmail.com
Signed-off-by: Peilin Ye <yepeilin.cs@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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Greg Kroah-Hartman]

On Wed, Aug 12, 2020 at 02:55:56AM -0400, Peilin Ye wrote:
> Prevent hfs_find_init() from dereferencing `tree` as NULL.
> 
> Reported-and-tested-by: syzbot+7ca256d0da4af073b2e2@syzkaller.appspotmail.com
> Signed-off-by: Peilin Ye <yepeilin.cs@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;
> +

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

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Peilin Ye]

On Wed, Aug 12, 2020 at 09:08:27AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 12, 2020 at 02:55:56AM -0400, Peilin Ye wrote:
> > Prevent hfs_find_init() from dereferencing `tree` as NULL.
> > 
> > Reported-and-tested-by: syzbot+7ca256d0da4af073b2e2@syzkaller.appspotmail.com
> > Signed-off-by: Peilin Ye <yepeilin.cs@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;
> > +
> 
> How can tree ever be NULL in these calls?  Shouldn't that be fixed as
> the root problem here?

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

Thank you,
Peilin Ye
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Greg Kroah-Hartman]

On Wed, Aug 12, 2020 at 03:13:06AM -0400, Peilin Ye wrote:
> On Wed, Aug 12, 2020 at 09:08:27AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Aug 12, 2020 at 02:55:56AM -0400, Peilin Ye wrote:
> > > Prevent hfs_find_init() from dereferencing `tree` as NULL.
> > > 
> > > Reported-and-tested-by: syzbot+7ca256d0da4af073b2e2@syzkaller.appspotmail.com
> > > Signed-off-by: Peilin Ye <yepeilin.cs@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;
> > > +
> > 
> > How can tree ever be NULL in these calls?  Shouldn't that be fixed as
> > the root problem here?
> 
> I see, I will try to figure out what is going on with the reproducer.

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Peilin Ye]

On Wed, Aug 12, 2020 at 10:18:52AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 12, 2020 at 03:13:06AM -0400, Peilin Ye wrote:
> > On Wed, Aug 12, 2020 at 09:08:27AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Aug 12, 2020 at 02:55:56AM -0400, Peilin Ye wrote:
> > > > Prevent hfs_find_init() from dereferencing `tree` as NULL.
> > > > 
> > > > Reported-and-tested-by: syzbot+7ca256d0da4af073b2e2@syzkaller.appspotmail.com
> > > > Signed-off-by: Peilin Ye <yepeilin.cs@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;
> > > > +
> > > 
> > > How can tree ever be NULL in these calls?  Shouldn't that be fixed as
> > > the root problem here?
> > 
> > I see, I will try to figure out what is going on with the reproducer.
> 
> 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?

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Big Budsupply]

[-- Attachment #1.1: Type: text/plain, Size: 1546 bytes --]

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 /Bigbudsupply1@gmail.com
Text/+14432672189
Looking forward to working with you guys

On Wed, 12 Aug 2020 at 09:59 Dan Carpenter <dan.carpenter@oracle.com> 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...  :/
>
>
>
> I wonder how long this has been broken and if we should just delete the
>
> AFS file system.
>
>
>
> regards,
>
> dan carpenter
>
>
>
> --
>
> 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-bugs+unsubscribe@googlegroups.com.
>
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/syzkaller-bugs/20200812085904.GA16441%40kadam
> .
>
>

[-- Attachment #1.2: Type: text/html, Size: 2537 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[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

> 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
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[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.

> I wonder how long this has been broken and if we should just delete the
> AFS file system.
> 
> regards,
> dan carpenter
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[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.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees