Messages in this thread | | | Subject | Re: [PATCH V2] hfs: do not free node before using | From | Viacheslav Dubeyko <> | Date | Sat, 24 Nov 2018 12:03:41 -0800 |
| |
On Sat, 2018-11-24 at 10:10 +0800, Pan Bian wrote: > The function hfs_bmap_free frees node via hfs_bnode_put(node). > However, > it then reads node->this when dumping error message on an error path, > which may result in a use-after-free bug. This patch frees node only > when it is never used. > > Fixes: a1185ffa2fc("HFS rewrite") > > Signed-off-by: Pan Bian <bianpan2016@163.com> > > --- > V2: correct the commit information in Fixes
By the way, HFS+ has the same issue [1].
> > --- > fs/hfs/btree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c > index 98b96ff..19017d2 100644 > --- a/fs/hfs/btree.c > +++ b/fs/hfs/btree.c > @@ -338,13 +338,14 @@ void hfs_bmap_free(struct hfs_bnode *node) > > nidx -= len * 8; > i = node->next; > - hfs_bnode_put(node); > if (!i) { > /* panic */; > pr_crit("unable to free bnode %u. bmap not > found!\n", > node->this); > + hfs_bnode_put(node); > return; > } > + hfs_bnode_put(node);
First of all, it looks weird to have two calls of hfs_bnode_put(node). Secondly, you will add only one line of code instead of two if you simply save the node->this in the local variable.
> node = hfs_bnode_find(tree, i); > if (IS_ERR(node)) > return;
Thanks, Vyacheslav Dubeyko.
[1] https://elixir.bootlin.com/linux/latest/source/fs/hfsplus/btree.c#L457
| |