[PATCH] hfsplus: Fix crash and filesystem corruption when deleting files

[PATCH] hfsplus: Fix crash and filesystem corruption when deleting files

[Simon Gander]

When removing files containing extended attributes, the hfsplus driver
may remove the wrong entries from the attributes b-tree, causing major
filesystem damage and in some cases even kernel crashes.

To remove a file, all its extended attributes have to be removed as well.
The driver does this by looking up all keys in the attributes b-tree with
the cnid of the file. Each of these entries then gets deleted using the
key used for searching, which doesn't contain the attribute's name when it
should. Since the key doesn't contain the name, the deletion routine will
not find the correct entry and instead remove the one in front of it. If
parent nodes have to be modified, these become corrupt as well. This causes
invalid links and unsorted entries that not even macOS's fsck_hfs is able
to fix.

To fix this, modify the search key before an entry is deleted from the
attributes b-tree by copying the found entry's key into the search key,
therefore ensuring that the correct entry gets removed from the tree.

Signed-off-by: Simon Gander <simon@tuxera.com>
Reviewed-by: Anton Altaparmakov <anton@tuxera.com>
---
 fs/hfsplus/attributes.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index e6d554476db4..eeebe80c6be4 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -292,6 +292,10 @@ static int __hfsplus_delete_attr(struct inode *inode, u32 cnid,
 		return -ENOENT;
 	}
 
+	/* Avoid btree corruption */
+	hfs_bnode_read(fd->bnode, fd->search_key,
+			fd->keyoffset, fd->keylength);
+
 	err = hfs_brec_remove(fd);
 	if (err)
 		return err;
-- 
2.26.0

Re: [PATCH] hfsplus: Fix crash and filesystem corruption when deleting files

[Andrew Morton]

On Fri, 27 Mar 2020 16:55:40 +0100 Simon Gander <simon@tuxera.com> wrote:

> When removing files containing extended attributes, the hfsplus driver
> may remove the wrong entries from the attributes b-tree, causing major
> filesystem damage and in some cases even kernel crashes.
> 
> To remove a file, all its extended attributes have to be removed as well.
> The driver does this by looking up all keys in the attributes b-tree with
> the cnid of the file. Each of these entries then gets deleted using the
> key used for searching, which doesn't contain the attribute's name when it
> should. Since the key doesn't contain the name, the deletion routine will
> not find the correct entry and instead remove the one in front of it. If
> parent nodes have to be modified, these become corrupt as well. This causes
> invalid links and unsorted entries that not even macOS's fsck_hfs is able
> to fix.
> 
> To fix this, modify the search key before an entry is deleted from the
> attributes b-tree by copying the found entry's key into the search key,
> therefore ensuring that the correct entry gets removed from the tree.
> 

This seems fairly important.  Should it have a cc:stable?

Re: [PATCH] hfsplus: Fix crash and filesystem corruption when deleting files

[Anton Altaparmakov]

Hi Andrew,

> On 28 Mar 2020, at 23:14, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 27 Mar 2020 16:55:40 +0100 Simon Gander <simon@tuxera.com> wrote:
>> When removing files containing extended attributes, the hfsplus driver
>> may remove the wrong entries from the attributes b-tree, causing major
>> filesystem damage and in some cases even kernel crashes.
>> 
>> To remove a file, all its extended attributes have to be removed as well.
>> The driver does this by looking up all keys in the attributes b-tree with
>> the cnid of the file. Each of these entries then gets deleted using the
>> key used for searching, which doesn't contain the attribute's name when it
>> should. Since the key doesn't contain the name, the deletion routine will
>> not find the correct entry and instead remove the one in front of it. If
>> parent nodes have to be modified, these become corrupt as well. This causes
>> invalid links and unsorted entries that not even macOS's fsck_hfs is able
>> to fix.
>> 
>> To fix this, modify the search key before an entry is deleted from the
>> attributes b-tree by copying the found entry's key into the search key,
>> therefore ensuring that the correct entry gets removed from the tree.
> 
> This seems fairly important.  Should it have a cc:stable?

It is - that is why we are pushing it upstream.  And yes, I think a cc:stable would be a good idea!

Best regards,

	Anton
-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer