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

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

[Ernesto A. Fernández]

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.

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.

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);
 again:
 	rec_off = tree->node_size - (fd->record + 2) * 2;
 	end_off = tree->node_size - (node->num_recs + 1) * 2;
-- 
2.1.4

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

[Viacheslav Dubeyko]

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?


> 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);
		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?
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?

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

Thanks,
Vyacheslav Dubeyko.

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

[Ernesto A. Fernández]

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

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

[Viacheslav Dubeyko]

On Sun, 2017-10-08 at 16:46 -0300, Ernesto A. Fernández wrote:
> 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?
> 

The script is the way to reproduce the issue. But the callstack of the
crash will be the explanation of the symptoms and the environment of
the issue. Also the callstack provides the opportunity to analyze the
issue.


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

I still cannot follow what the real cause of the issue. This is why I
requested the callstack of the crash. Could you point out the code line
in hfs_brec_remove() that is the place of the crash? I don't see the
place where hfs_brec_remove() is trying to access the fd->search_key
directly. It sounds for me that the real trouble is not in the
hfs_brec_remove().

Let's imagine that we have trouble with fd->search_key in the
hfs_brec_remove(). But then it means that the real fix should be
in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am
not convinced by your explanation that the reason is there where you
did fix it.

The struct hfsplus_attr_key is simply the piece of memory:

struct hfsplus_attr_key {
	__be16 key_len;
	__be16 pad;
	hfsplus_cnid cnid;
	__be32 start_block;
	struct hfsplus_attr_unistr key_name;
} __packed;

And the hfsplus_attr_build_key() initializes this piece of memory:

int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
			u32 cnid, const char *name)
{
	int len;

	memset(key, 0, sizeof(struct hfsplus_attr_key));
	key->attr.cnid = cpu_to_be32(cnid);
	if (name) {
		int res = hfsplus_asc2uni(sb,
				(struct hfsplus_unistr *)&key->attr.key_name,
				HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
		if (res)
			return res;
		len = be16_to_cpu(key->attr.key_name.length);
	} else {
		key->attr.key_name.length = 0;
		len = 0;
	}

	/* The length of the key, as stored in key_len field, does not include
	 * the size of the key_len field itself.
	 * So, offsetof(hfsplus_attr_key, key_name) is a trick because
	 * it takes into consideration key_len field (__be16) of
	 * hfsplus_attr_key structure instead of length field (__be16) of
	 * hfsplus_attr_unistr structure.
	 */
	key->key_len =
		cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
				2 * len);

	return 0;
}

So, even if we haven't name in this structure then the attr.key_name
will have zero length and the buffer will contain the string is filled
by zeros. And it's not clear for me how this can be the reason of
crash? Could you share the callstack of the crash, finally? I still
don't follow your vision. Please, explain the issue and your fix in
more clear and detailed way.

It is possible to do the search by name only or by cnid only.
Everything should work OK. And I didn't rememer any troubles
with hfsplus_delete_all_attrs() on the implementation and testing
phase. I think you could have some specific environment or maybe the
code base was changed dramatically. But, anyway, I don't follow your
vision of the issue.

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

The using of sizeof(hfs_bnode_desc) will be pretty good and clean
solution.

Thanks,
Vyacheslav Dubeyko.

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

[Ernesto A. Fernández]

On Mon, Oct 09, 2017 at 10:03:50AM -0700, Viacheslav Dubeyko wrote:
> On Sun, 2017-10-08 at 16:46 -0300, Ernesto A. Fernández wrote:
> > 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?
> > 
> 
> The script is the way to reproduce the issue. But the callstack of the
> crash will be the explanation of the symptoms and the environment of
> the issue. Also the callstack provides the opportunity to analyze the
> issue.

All right, I'll include it at the end of the mail.

> > > 
> > > 
> > > > 
> > > > 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.
> > 
> 
> I still cannot follow what the real cause of the issue. This is why I
> requested the callstack of the crash. Could you point out the code line
> in hfs_brec_remove() that is the place of the crash?

Sure, the crash happens in line 219, when the out of bounds write
takes place. But that doesn't tell you much, because the real issue
here is that fd->record has been set to -1 before this call. That value
means that no record has been found, but since that makes no sense (in
this context) the hfs_brec_remove() code acts as if it was an actual
record and tries to write to it.

> I don't see the
> place where hfs_brec_remove() is trying to access the fd->search_key
> directly. It sounds for me that the real trouble is not in the
> hfs_brec_remove().

It is used directly in line 206, explicitly searching the parent node by
key when the search_key has not been set for that purpose. It also uses
it indirectly in line 229, again to search the parent records. In the
second case this just means that we will fail to update the parent record,
which is bad enough. But the first case is the one that will set fd->record
to -1 before going through the code again, this time for the parent node.
When hfs_bnode_write_u16() tries to delete this "-1 record" on the parent,
there is an out of bounds write.

This last thing only happens when we have just deleted the last record of a
bnode.

> Let's imagine that we have trouble with fd->search_key in the
> hfs_brec_remove(). But then it means that the real fix should be
> in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am
> not convinced by your explanation that the reason is there where you
> did fix it.

I thought about having hfs_brec_find() set fd->search_key to the
same value as fd->key before returning, but I think that would be wrong.
The caller may expect the search_key to remain the same after the search,
and even reuse it. In fact that's what the hfs_brec_remove() function is
trying to do, in a buggy style.

> The struct hfsplus_attr_key is simply the piece of memory:
> 
> struct hfsplus_attr_key {
> 	__be16 key_len;
> 	__be16 pad;
> 	hfsplus_cnid cnid;
> 	__be32 start_block;
> 	struct hfsplus_attr_unistr key_name;
> } __packed;
> 
> And the hfsplus_attr_build_key() initializes this piece of memory:
> 
> int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
> 			u32 cnid, const char *name)
> {
> 	int len;
> 
> 	memset(key, 0, sizeof(struct hfsplus_attr_key));
> 	key->attr.cnid = cpu_to_be32(cnid);
> 	if (name) {
> 		int res = hfsplus_asc2uni(sb,
> 				(struct hfsplus_unistr *)&key->attr.key_name,
> 				HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
> 		if (res)
> 			return res;
> 		len = be16_to_cpu(key->attr.key_name.length);
> 	} else {
> 		key->attr.key_name.length = 0;
> 		len = 0;
> 	}
> 
> 	/* The length of the key, as stored in key_len field, does not include
> 	 * the size of the key_len field itself.
> 	 * So, offsetof(hfsplus_attr_key, key_name) is a trick because
> 	 * it takes into consideration key_len field (__be16) of
> 	 * hfsplus_attr_key structure instead of length field (__be16) of
> 	 * hfsplus_attr_unistr structure.
> 	 */
> 	key->key_len =
> 		cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
> 				2 * len);
> 
> 	return 0;
> }
> 
> So, even if we haven't name in this structure then the attr.key_name
> will have zero length and the buffer will contain the string is filled
> by zeros. And it's not clear for me how this can be the reason of
> crash? Could you share the callstack of the crash, finally? I still
> don't follow your vision. Please, explain the issue and your fix in
> more clear and detailed way.

I haven't checked the key comparison function used, but what I see in
practice is that when you use a search_key that was set in this way
(prepared only to find the first record with a given cnid) for the purpose
of a search by key, the result is not correct. I get fd->record == -1
in my case, but maybe it can fail in different ways in other situations.
I thought this was clearly a bug in the caller, that had not set the key
to the right value. You seem to think that the search should work anyway,
I believe?

> 
> It is possible to do the search by name only or by cnid only.
> Everything should work OK.

I don't think you can search by name only. You can search by key, and the
key has both the name and the cnid. The search by cnid is for finding the
first record with a given cnid, you don't yet know the name so you can't
search by key. This is used by hfsplus_delete_all_attrs() because it
doesn't care about which record is deleted first, it simply deletes all
with a given cnid.

Also you have to explicitly state in the call to __hfs_brec_find() which
one of the two search strategies you will be using. I don't think it is
reasonable to expect __hfs_brec_find() to give a valid result if it is
called with hfs_find_rec_by_key but fd->search_key is only prepared for
a search by cnid.

> And I didn't rememer any troubles
> with hfsplus_delete_all_attrs() on the implementation and testing
> phase. I think you could have some specific environment or maybe the
> code base was changed dramatically. But, anyway, I don't follow your
> vision of the issue.

You are probably right here: I've done all my testing in a fresh
filesystem, and I realize now this segfault would be harder to trigger
if the filesystem has been aged, because the xattrs of different files
may share a single bnode. So deleting all xattrs of a single file would
not necessarily delete a parent bnode, no matter how many xattrs.

This bug would still have consequences though, but they may be more
subtle.

> > > 
> > > 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.
> > 
> 
> The using of sizeof(hfs_bnode_desc) will be pretty good and clean
> solution.
> 
> Thanks,
> Vyacheslav Dubeyko.
> 
> 
> 

Here's the callstack, I hope it helps you:

[ 3550.503259] general protection fault: 0000 [#1] SMP
[ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy
[ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16
[ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000
[ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus]
[ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202
[ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff
[ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780
[ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000
[ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002
[ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56
[ 3550.504013] FS:  00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
[ 3550.504013] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0
[ 3550.504013] Call Trace:
[ 3550.504013]  hfsplus_bnode_write_u16+0x27/0x30 [hfsplus]
[ 3550.504013]  hfsplus_brec_remove+0x117/0x170 [hfsplus]
[ 3550.504013]  __hfsplus_delete_attr+0x94/0xf0 [hfsplus]
[ 3550.504013]  hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus]
[ 3550.504013]  hfsplus_delete_cat+0x1f5/0x300 [hfsplus]
[ 3550.504013]  hfsplus_unlink+0x82/0x1e0 [hfsplus]
[ 3550.504013]  ? __inode_permission+0x44/0xc0
[ 3550.504013]  vfs_unlink+0xf1/0x180
[ 3550.504013]  do_unlinkat+0x25f/0x2e0
[ 3550.504013]  SyS_unlinkat+0x1b/0x30
[ 3550.504013]  entry_SYSCALL_64_fastpath+0x1e/0xa9
[ 3550.504013] RIP: 0033:0x7fc27ebe632d
[ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107
[ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d
[ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c
[ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000
[ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030
[ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000
[ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48
[ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08
[ 3550.540759] ---[ end trace 142de398139577f1 ]---

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

[Viacheslav Dubeyko]

On Mon, 2017-10-09 at 16:59 -0300, Ernesto A. Fernández wrote:

> > 
> > 
> > 
> Here's the callstack, I hope it helps you:
> 
> [ 3550.503259] general protection fault: 0000 [#1] SMP
> [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd
> auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm
> configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr
> evdev parport_pc parport pvpanic serio_raw drm_kms_helper button
> 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom
> ata_generic ata_piix libata crc32c_intel psmouse virtio_pci
> virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy
> [ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16
> [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000
> [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus]
> [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202
> [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX:
> 00000000000000ff
> [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI:
> ffff88002972d780
> [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09:
> 0000000000000000
> [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12:
> 0000000000000002
> [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15:
> ffffc90001633c56
> [ 3550.504013] FS:  00007fc27f0c8700(0000) GS:ffff88007fc00000(0000)
> knlGS:0000000000000000
> [ 3550.504013] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4:
> 00000000000006f0
> [ 3550.504013] Call Trace:
> [ 3550.504013]  hfsplus_bnode_write_u16+0x27/0x30 [hfsplus]
> [ 3550.504013]  hfsplus_brec_remove+0x117/0x170 [hfsplus]
> [ 3550.504013]  __hfsplus_delete_attr+0x94/0xf0 [hfsplus]
> [ 3550.504013]  hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus]
> [ 3550.504013]  hfsplus_delete_cat+0x1f5/0x300 [hfsplus]
> [ 3550.504013]  hfsplus_unlink+0x82/0x1e0 [hfsplus]
> [ 3550.504013]  ? __inode_permission+0x44/0xc0
> [ 3550.504013]  vfs_unlink+0xf1/0x180
> [ 3550.504013]  do_unlinkat+0x25f/0x2e0
> [ 3550.504013]  SyS_unlinkat+0x1b/0x30
> [ 3550.504013]  entry_SYSCALL_64_fastpath+0x1e/0xa9
> [ 3550.504013] RIP: 0033:0x7fc27ebe632d
> [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX:
> 0000000000000107
> [ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX:
> 00007fc27ebe632d
> [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI:
> ffffffffffffff9c
> [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09:
> 0000000000000000
> [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12:
> 00000000025cc030
> [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15:
> 0000000000000000
> [ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0
> 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6
> 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49
> 8b 0f 48
> [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP:
> ffffc90001633c08
> [ 3550.540759] ---[ end trace 142de398139577f1 ]---

Great. Thank you.

I can reproduce the crash. Let me check the issue.

Thanks,
Vyacheslav Dubeyko.

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

[Slava Dubeyko]

-----Original Message-----
From: Ernesto A. Fernández [mailto:ernesto.mnd.fernandez@gmail.com] 
Sent: Monday, October 9, 2017 1:00 PM
To: Viacheslav Dubeyko <slava@dubeyko.com>; linux-fsdevel@vger.kernel.org
Cc: Sergei Antonov <saproj@gmail.com>; Hin-Tak Leung <htl10@users.sourceforge.net>; Al Viro <viro@zeniv.linux.org.uk>; Christoph Hellwig <hch@infradead.org>; Slava Dubeyko <Vyacheslav.Dubeyko@wdc.com>; Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
Subject: Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file

> Here's the callstack, I hope it helps you:
> 
> [ 3550.503259] general protection fault: 0000 [#1] SMP
> [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy
> [ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16
> [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000
> [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus]
> [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202
> [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff
> [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780
> [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000
> [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002
> [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56
> [ 3550.504013] FS:  00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
> [ 3550.504013] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0
> [ 3550.504013] Call Trace:
> [ 3550.504013]  hfsplus_bnode_write_u16+0x27/0x30 [hfsplus]
> [ 3550.504013]  hfsplus_brec_remove+0x117/0x170 [hfsplus]
> [ 3550.504013]  __hfsplus_delete_attr+0x94/0xf0 [hfsplus]
> [ 3550.504013]  hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus]
> [ 3550.504013]  hfsplus_delete_cat+0x1f5/0x300 [hfsplus]
> [ 3550.504013]  hfsplus_unlink+0x82/0x1e0 [hfsplus]
> [ 3550.504013]  ? __inode_permission+0x44/0xc0
> [ 3550.504013]  vfs_unlink+0xf1/0x180
> [ 3550.504013]  do_unlinkat+0x25f/0x2e0
> [ 3550.504013]  SyS_unlinkat+0x1b/0x30
> [ 3550.504013]  entry_SYSCALL_64_fastpath+0x1e/0xa9
> [ 3550.504013] RIP: 0033:0x7fc27ebe632d
> [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107
> [ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d
> [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c
> [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000
> [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030
> [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000
> [ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48
> [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08
> [ 3550.540759] ---[ end trace 142de398139577f1 ]---
> 

The whole picture is here:

(1) hfsplus_find_attr()
 [  458.039682] hfsplus: find_attr: (null),18

(2) hfs_brec_find(fd, hfs_find_1st_rec_by_cnid)
 [  458.039683] hfsplus: put_node(8:1): 1
 [  458.039685] hfsplus: get_node(8:3): 82
 [  458.039686] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4
 [  458.039687] hfsplus: put_node(8:3): 82
 [  458.039688] hfsplus: get_node(8:1): 1
 [  458.039690] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 30, fd->entryoffset 44, fd->entrylength 16

(3) __hfsplus_delete_attr()
(4) hfs_brec_remove()
 [  458.039691] hfsplus: bnode: 1
 [  458.039692] hfsplus: 2, 0, -1, 1, 1
 [  458.039693] hfsplus:  14 (28)
 [  458.039695] hfsplus:  60
 [  458.039697] hfsplus: remove_rec: 0, 46
 [  458.039697] hfsplus: remove_rec: num_recs 1
 [  458.039700] hfsplus: new_node(8:2): 1
 [  458.040614] hfsplus: put_node(8:2): 1
 [  458.040618] hfsplus: get_node(8:3): 82
 [  458.040619] hfsplus: put_node(8:1): 1
 [  458.040620] hfsplus: remove_node(8:1): 0
 [  458.040622] hfsplus: btree_free_node: 1
 [  458.040623] hfsplus: new_node(8:0): 1
 [  458.041096] hfsplus: put_node(8:0): 1
 [  458.041102] hfsplus: __hfs_brec_find: fd->record -1, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4
 [  458.041103] hfsplus: bnode: 3
 [  458.041104] hfsplus: 0, 0, 0, 2, 2
 [  458.041105] hfsplus:  14 (26,1)
 [  458.041107] hfsplus:  44 (30,2)
 [  458.041110] hfsplus:  78
 [  458.041112] hfsplus: remove_rec: -1, 30
 [  458.041113] hfsplus: remove_rec: num_recs 2
 [  458.041114] hfsplus: remove_rec: num_recs 1
 [  458.041117] hfsplus: remove_rec: rec_off 8190, end_off 8186
 [  458.041118] hfsplus: remove_rec: rec_off 8190, end_off 8186
 [  458.041119] hfsplus: remove_rec: data_off 14, size 30
 [  458.041143] general protection fault: 0000 [#1] SMP

The hfs_brec_find() found the pointer (#1) in index node #3 by means of hfs_find_1st_rec_by_cnid strategy.
Then the hfs_brec_find() found the single record in the leaf node #1 on the basis of the same strategy.
The most interesting is starting in hfs_brec_remove():

    if (!--node->num_recs) {
	hfs_bnode_unlink(node);
	if (!node->parent)
	    return 0;
	parent = hfs_bnode_find(tree, node->parent);
	if (IS_ERR(parent))
	    return PTR_ERR(parent);
	hfs_bnode_put(node);
	node = fd->bnode = parent;

	__hfs_brec_find(node, fd, hfs_find_rec_by_key);
	goto again;
    }

The hfs_brec_remove() is trying to delete the single and latest record from the leaf node #1.
Finally, the node->num_recs will be equal to zero after the increment operation and
the condition directs the code inside of this execution path. So, first of all, the logic
unlink the leaf node #2 from leaf node #1. Then the logic gets the parent node and to put
the leaf node #1. And, finally, we are trying to find a record in the parent and index
node #3 on the basis of hfs_find_rec_by_key strategy. We cannot find anything because
the fd->search_key is not prepared for such search. It means that fd->record is equal to
-1. The real issue takes place here:

hfs_bnode_write_u16(node, rec_off + 2, data_off - size);

because the data_off == 14 and size == 30. We have overflow here and it is the reason
of the crash. But the data_off is lesser of size because of value of fd->record we
selected improper position of the offset for the changing the value in the offsets table.

What should be done for the issue fix?
(1) We need to add the check condition here:

http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L217

    do {
	data_off = hfs_bnode_read_u16(node, rec_off);
/*
 * TODO: check condition here
 *       if (data_off < size)
 *           return error here!!!!!
 */
	hfs_bnode_write_u16(node, rec_off + 2, data_off - size);
	rec_off -= 2;
    } while (rec_off >= end_off);

(2) The logic of hfs_brec_remove() should be reworked here:

http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L197

/*
 * TODO: rework the logic here

    if (node->num_recs <= 0)
	BUG(); /* it could be error message here */
    else if (node->num_recs == 1)
	__hfs_brec_find(node, fd, hfs_find_1st_rec_by_cnid); /* maybe check the error here */
	memcpy(fd->search_key, fd->key, sizeof(hfsplus_btree_key));
	--node->num_recs;
 */

	hfs_bnode_unlink(node);
	if (!node->parent)
	    return 0;
	parent = hfs_bnode_find(tree, node->parent);
	if (IS_ERR(parent))
	    return PTR_ERR(parent);
	hfs_bnode_put(node);
	node = fd->bnode = parent;

	__hfs_brec_find(node, fd, hfs_find_rec_by_key);
	goto again;
    }

So, we need to extract the key of the latest record from the leaf node by using
the hfs_find_1st_rec_by_cnid strategy, copy fd->key into the fd->search_key
and then to go into logic of searching the record in the index node on the basis
of hfs_find_rec_by_key strategy and the deletion of the record from the index node.

What do you think? Could you rework the patch?

Thanks,
Vyacheslav Dubeyko.

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

[Ernesto A. Fernández]

I really appreciate your feedback, but I would rather you spent more
time trying to understand my work instead of redoing it from scratch.
You are making many of the same mistakes I made when I was first tracking
down this bug:

On Tue, Oct 10, 2017 at 09:39:08PM +0000, Slava Dubeyko wrote:
> The hfs_brec_find() found the pointer (#1) in index node #3 by means of hfs_find_1st_rec_by_cnid strategy.
> Then the hfs_brec_find() found the single record in the leaf node #1 on the basis of the same strategy.
> The most interesting is starting in hfs_brec_remove():
> 
>     if (!--node->num_recs) {
> 	hfs_bnode_unlink(node);
> 	if (!node->parent)
> 	    return 0;
> 	parent = hfs_bnode_find(tree, node->parent);
> 	if (IS_ERR(parent))
> 	    return PTR_ERR(parent);
> 	hfs_bnode_put(node);
> 	node = fd->bnode = parent;
> 
> 	__hfs_brec_find(node, fd, hfs_find_rec_by_key);
> 	goto again;
>     }
> 
> The hfs_brec_remove() is trying to delete the single and latest record from the leaf node #1.
> Finally, the node->num_recs will be equal to zero after the increment operation and
> the condition directs the code inside of this execution path. So, first of all, the logic
> unlink the leaf node #2 from leaf node #1. Then the logic gets the parent node and to put
> the leaf node #1. And, finally, we are trying to find a record in the parent and index
> node #3 on the basis of hfs_find_rec_by_key strategy. We cannot find anything because
> the fd->search_key is not prepared for such search. It means that fd->record is equal to
> -1. The real issue takes place here:
> 
> hfs_bnode_write_u16(node, rec_off + 2, data_off - size);
> 
> because the data_off == 14 and size == 30. We have overflow here and it is the reason
> of the crash. 

No. The reason of the crash is that  rec_off + 2  is an offset outside
the limits of the bnode, and we are writing to it. That's how you get a
segfault.

data_off has become 14 because that is the value that is always saved in
the last two bytes of the bnode. It is the position of the first record,
the weird contant you didn't like in my patch. Now of course, this is
wrong, the last two bytes of the bnode should not have been read at all
here:

  data_off = hfs_bnode_read_u16(node, rec_off);

because rec_off should have been two bytes to the left. But that's a
separate consequence of the fact that fd->record should never have been
-1 in the first place.

The fact is, size will always be smaller than data_off if the code has no
bugs. In fact, when we are deleting the first record of the bnode,
data_off - size will be exactly 14 in the first iteration, which is what
we want stored in the last two bytes.

> But the data_off is lesser of size because of value of fd->record we
> selected improper position of the offset for the changing the value in the offsets table.
> 
> What should be done for the issue fix?
> (1) We need to add the check condition here:
> 
> http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L217
> 
>     do {
> 	data_off = hfs_bnode_read_u16(node, rec_off);
> /*
>  * TODO: check condition here
>  *       if (data_off < size)

Like I said above, this will never happen if the code is correct, so
the logical response here would be BUG(). Think about it, why would the
offset of a record that comes after the one we are deleting be smaller
than the size of the one we are deleting?

I really don't see any point in having this check at all, though.

>  *           return error here!!!!!
>  */
> 	hfs_bnode_write_u16(node, rec_off + 2, data_off - size);
> 	rec_off -= 2;
>     } while (rec_off >= end_off);
> 
> (2) The logic of hfs_brec_remove() should be reworked here:
> 
> http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L197
> 
> /*
>  * TODO: rework the logic here
> 
>     if (node->num_recs <= 0)
> 	BUG(); /* it could be error message here */
>     else if (node->num_recs == 1)
> 	__hfs_brec_find(node, fd, hfs_find_1st_rec_by_cnid); /* maybe check the error here */

Why are you searching the bnode if you only have one record left? That's
the one you want. And fd->key is alredy set to it. Also no, there is no
need to check the error, we shouldn't be here at all if there was no
record with that cnid.

> 	memcpy(fd->search_key, fd->key, sizeof(hfsplus_btree_key));

This line is the actual fix (close enough to the one in my patch), but
it's in the wrong place. If you read my last mail (which I have the
feeling you skipped) you'll know that this bug also touches the call
to hfs_brec_update_parent() in line 229. So with your solution there
will be no segfault, but the parent records won't be updated at all
unless they need to be removed.

Also, why are you copying the whole of the hfsplus_btree_key structure,
which is like 300 bytes long, when the valid data is usually only in the
first 30 bytes or so? I don't think it will make a difference but it seems
wasteful, particularly compared to my patch which made use of the
existing infrastructure to handle variable length keys.

> 	--node->num_recs;
>  */
> 
> 	hfs_bnode_unlink(node);
> 	if (!node->parent)
> 	    return 0;
> 	parent = hfs_bnode_find(tree, node->parent);
> 	if (IS_ERR(parent))
> 	    return PTR_ERR(parent);
> 	hfs_bnode_put(node);
> 	node = fd->bnode = parent;
> 
> 	__hfs_brec_find(node, fd, hfs_find_rec_by_key);
> 	goto again;
>     }
> 
> So, we need to extract the key of the latest record from the leaf node by using
> the hfs_find_1st_rec_by_cnid strategy,

Not really. The key of the parent record is also the key of the first
record of the child, so you can just copy that, like my patch does.

> copy fd->key into the fd->search_key
> and then to go into logic of searching the record in the index node on the basis
> of hfs_find_rec_by_key strategy and the deletion of the record from the index node.
> 
> What do you think? Could you rework the patch?

Your solution is oddly convoluted, and is equal to my one-line fix in
the one thing it does that matters. Yet it still fails to solve half
the issue. So I have to say no here.

You can't expect to make a correct patch by just going through callstacks
and debug info; that's just dealing with the symptoms. To fix this for
real you have to actually figure out what the code is doing.

> 
> Thanks,
> Vyacheslav Dubeyko.
> 

Now that you've done your own research: could you please go back to my
patch and point out, with your improved understanding of the issue, what
it is exactly that you think is wrong with it?

This module does not seem to be getting a lot of attention lately, so I'm
very afraid of introducing some nasty bug that will remain in the code
for years. I will very much appreciate a careful review.

Ernest

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

[Hin-Tak Leung]

--------------------------------------------
On Sat, 7/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> 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.

Sounds like it is easily/reliably reproducible - do you have an estimate/recipe of how many xattrs minimum to trigger this problem, to test and verify this fix?

Thanks a lot for the work!

Hin-Tak

 

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

[Ernesto A. Fernández]

On Sat, Oct 07, 2017 at 04:25:22AM +0000, Hin-Tak Leung wrote:
> 
> --------------------------------------------
> On Sat, 7/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> 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.
> 
> Sounds like it is easily/reliably reproducible - do you have an estimate/recipe of how many xattrs minimum to trigger this problem, to test and verify this fix?

I did not give a number because it depends on the size of the xattrs, and I
imagine the bnode size as well, though I only tested it with the default
mkfs options. Maybe I should have shared the script I used instead:

touch test.file
i=1
while [ $i -le 250 ]; do
  setfattr -n user.$i test.file
  ((++i))
done
rm test.file

Of course, if you set a value to the xattrs you will need fewer.

> Thanks a lot for the work!

And thank you for your attention.

> Hin-Tak
> 
>  

Ernest

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

[Hin-Tak Leung]

--------------------------------------------
On Sun, 8/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> wrote:

> I did not give a number because it depends on the size of the xattrs, and I
> imagine the bnode size as well, though I only tested it with the default
> mkfs options. Maybe I should have shared the script I used instead:

> touch test.file
> i=1
> while [ $i -le 250 ]; do
>  setfattr -n user.$i test.file
>  ((++i))
> done
> rm test.file

> Of course, if you set a value to the xattrs you will need fewer.

Yes, including the script in the commit message would be useful; for just the case of default mkfs options is perfectly acceptable. Definitely good to include it in the next revision of your patch. Don't worry about non-default/existing xattrs, or exactly how many is needed; a reliable way (3 times out of 4 say, or repeated running 2-3 times) under default condition is good enough to include.

Hin-Tak