Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

On Fri, Oct 06, 2017 at 10:03:01PM -0700, Viacheslav Dubeyko wrote:
> On Fri, 2017-10-06 at 18:52 -0300, Ernesto A. Fernández wrote:
> > A segmentation fault can be triggered by setting many xattrs to a
> > file
> > and then deleting it. The number must be high enough for more than
> > one
> > b-tree node to be needed for storage.
> > 
> 
> I think it could be great to see in the description:
> (1) The explanation of the reproducing path of the issue;
> (2) The callstack of the crash and more details about it (if you are
> able to reproduce the issue, of course).
> 
> Could you please add these details in the description?

I sent the script I used in a separate mail, would that be enough help?
It seems from your review that you already understood all that the
callstack can tell you. Maybe I should add the script to the commit
message?

> 
> > When hfs_brec_remove() is called as part of
> > hfsplus_delete_all_attrs(),
> > fd->search_key will not be set to any specific value. It does not
> > matter
> > because we intend to remove all records for a given cnid.
> 
> 
> I am not fully follow to the issue. The hfsplus_delete_all_attrs calls
> hfsplus_find_attr() before hfsplus_delete_attr():
> 
> 	for (;;) {
> 		err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd);
> 		if (err) {
> 			if (err != -ENOENT)
> 				pr_err("xattr search failed\n");
> 			goto end_delete_all;
> 		}
> 
> 		err = __hfsplus_delete_attr(dir, cnid, &fd);
> 		if (err)
> 			goto end_delete_all;
> 	}
> 
> The hfsplus_find_attr() prepares fd->search_key:
> 
> 	if (name) {
> 		err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name);
> 		if (err)
> 			goto failed_find_attr;
> 		err = hfs_brec_find(fd, hfs_find_rec_by_key);
> 		if (err)
> 			goto failed_find_attr;
> 	} else {
> 		err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL);

Here hfsplus_attr_build_key will only prepare the key for a search by cnid.
We do not yet know the name of the xattr (that's what the NULL is) so it
can't be set to anything specific.

> 		if (err)
> 			goto failed_find_attr;
> 		err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid);
> 		if (err)
> 			goto failed_find_attr;
> 	}
> 
> And, finally, __hfsplus_delete_attr() calls the hfs_brec_remove(). I am
> not follow to the explanation of the issue. Maybe I missed something?

The problem is that hfs_brec_remove() assumes that fd was prepared for a
search by key, but it was prepared for a search by cnid. It will fail to
find the parent records.

> Could you share the callstack of the crash andd more details?
> 
> > 
> > The problem is that hfs_brec_remove() assumes it is being called with
> > the result of a search by key, not by cnid. The value of search_key
> > may
> > be used to update the parent nodes. When no appropriate parent record
> > is
> > found, the result is an out of bounds access.
> > 
> > To fix this, set the value of fd->search_key to the key of the first
> > record in the node, which is also the key of the corresponding parent
> > record.
> > 
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> > ---
> >  fs/hfsplus/brec.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> > index 754fdf8..dfa60cf 100644
> > --- a/fs/hfsplus/brec.c
> > +++ b/fs/hfsplus/brec.c
> > @@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd)
> >  
> >  	tree = fd->tree;
> >  	node = fd->bnode;
> > +
> > +	/* in case we need to search the parent node */
> > +	hfs_bnode_read_key(node, fd->search_key, 14);
> 
> The hardcoded value looks really weird. Could you rework this by means
> of adding some constant with reasonable name? Why 14?

I agree that it's weird, but the number 14 is all over brec.c. It would be
more confusing if I used a constant only this one time, so I decided to
respect the style that was in use.

To be clear, 14 is sizeof(hfs_bnode_desc), that is, the size of the
header of the bnode, and the position of the first record. If you find
it better, maybe we can change 14 to sizeof(hfs_bnode_desc) every
time it appears in the file, but I think it should be a separate patch.

> 
> >  again:
> >  	rec_off = tree->node_size - (fd->record + 2) * 2;
> >  	end_off = tree->node_size - (node->num_recs + 1) * 2;
> 
> Thanks,
> Vyacheslav Dubeyko.
> 
> 

Thank you for your review,
Ernest