Prevent hfs_find_init() from dereferencing `tree` as NULL.
Reported-and-tested-by: [email protected]
Signed-off-by: Peilin Ye <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> 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
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: [email protected]
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > 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
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: [email protected]
> > > Signed-off-by: Peilin Ye <[email protected]>
> > > ---
> > > 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
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
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: [email protected]
> > > > Signed-off-by: Peilin Ye <[email protected]>
> > > > ---
> > > > 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
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
>
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
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.