| Message ID | 20200812065556.869508-1-yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Linux-kernel-mentees] hfs, hfsplus: Fix NULL pointer dereference in hfs_find_init() | expand |
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
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
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
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: 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
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.
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);
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(+)