[PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[Sasha Levin]

From: Linus Torvalds <torvalds@linux-foundation.org>

[ Upstream commit f3bbac32475b27f49be201f896d98d4009de1562 ]

We potentially have old hashes of the xattr names generated on systems
with signed 'char' types.  Now that everybody uses '-funsigned-char',
those hashes will no longer match.

This only happens if you use xattrs names that have the high bit set,
which probably doesn't happen in practice, but the xfstest generic/454
shows it.

Instead of adding a new "signed xattr hash filesystem" bit and having to
deal with all the possible combinations, just calculate the hash both
ways if the first one fails, and always generate new hashes with the
proper unsigned char version.

Reported-by: kernel test robot <oliver.sang@intel.com>
Link: https://lore.kernel.org/oe-lkp/202212291509.704a11c9-oliver.sang@intel.com
Link: https://lore.kernel.org/all/CAHk-=whUNjwqZXa-MH9KMmc_CpQpoFKFjAB9ZKHuu=TbsouT4A@mail.gmail.com/
Exposed-by: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Andreas Dilger <adilger@dilger.ca>
Cc: Theodore Ts'o <tytso@mit.edu>,
Cc: Jason Donenfeld <Jason@zx2c4.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/ext4/xattr.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 866772a2e068..5f57a8addcbc 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -81,6 +81,8 @@ ext4_xattr_block_cache_find(struct inode *, struct ext4_xattr_header *,
 			    struct mb_cache_entry **);
 static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
 				    size_t value_count);
+static __le32 ext4_xattr_hash_entry_signed(char *name, size_t name_len, __le32 *value,
+				    size_t value_count);
 static void ext4_xattr_rehash(struct ext4_xattr_header *);
 
 static const struct xattr_handler * const ext4_xattr_handler_map[] = {
@@ -470,8 +472,21 @@ ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
 		tmp_data = cpu_to_le32(hash);
 		e_hash = ext4_xattr_hash_entry(entry->e_name, entry->e_name_len,
 					       &tmp_data, 1);
-		if (e_hash != entry->e_hash)
-			return -EFSCORRUPTED;
+		/* All good? */
+		if (e_hash == entry->e_hash)
+			return 0;
+
+		/*
+		 * Not good. Maybe the entry hash was calculated
+		 * using the buggy signed char version?
+		 */
+		e_hash = ext4_xattr_hash_entry_signed(entry->e_name, entry->e_name_len,
+							&tmp_data, 1);
+		if (e_hash == entry->e_hash)
+			return 0;
+
+		/* Still no match - bad */
+		return -EFSCORRUPTED;
 	}
 	return 0;
 }
@@ -3090,6 +3105,28 @@ static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
 	return cpu_to_le32(hash);
 }
 
+/*
+ * ext4_xattr_hash_entry_signed()
+ *
+ * Compute the hash of an extended attribute incorrectly.
+ */
+static __le32 ext4_xattr_hash_entry_signed(char *name, size_t name_len, __le32 *value, size_t value_count)
+{
+	__u32 hash = 0;
+
+	while (name_len--) {
+		hash = (hash << NAME_HASH_SHIFT) ^
+		       (hash >> (8*sizeof(hash) - NAME_HASH_SHIFT)) ^
+		       (signed char)*name++;
+	}
+	while (value_count--) {
+		hash = (hash << VALUE_HASH_SHIFT) ^
+		       (hash >> (8*sizeof(hash) - VALUE_HASH_SHIFT)) ^
+		       le32_to_cpu(*value++);
+	}
+	return cpu_to_le32(hash);
+}
+
 #undef NAME_HASH_SHIFT
 #undef VALUE_HASH_SHIFT
 
-- 
2.39.0

Re: [PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[Linus Torvalds]

On Tue, Jan 24, 2023 at 5:42 AM Sasha Levin <sashal@kernel.org> wrote:
>
> From: Linus Torvalds <torvalds@linux-foundation.org>
>
> [ Upstream commit f3bbac32475b27f49be201f896d98d4009de1562 ]
>
> We potentially have old hashes of the xattr names generated on systems
> with signed 'char' types.  Now that everybody uses '-funsigned-char',
> those hashes will no longer match.

This patch does not work correctly without '-funsigned-char', and I
don't think that has been back-ported to stable kernels.

That said, the patch *almost* works. You'd just have to add something
like this to it:

  --- a/fs/ext4/xattr.c
  +++ b/fs/ext4/xattr.c
  @@ -3096,7 +3096,7 @@ static __le32 ext4_xattr_hash_entry(char *name,
        while (name_len--) {
                hash = (hash << NAME_HASH_SHIFT) ^
                       (hash >> (8*sizeof(hash) - NAME_HASH_SHIFT)) ^
  -                    *name++;
  +                    (unsigned char)*name++;
        }
        while (value_count--) {
                hash = (hash << VALUE_HASH_SHIFT) ^

to make it work right (ie just make sure that the proper xattr name
hashing actually uses unsigned chars for its hash).

                    Linus

Re: [PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Tue, Jan 24, 2023 at 8:50 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This patch does not work correctly without '-funsigned-char', and I
> don't think that has been back-ported to stable kernels.
>
> That said, the patch *almost* works.

So I'm  not convinced this should be back-ported at all, but it's
certainly true that going back and forth between the two cases would
be problematic.

Maybe the right thing to do would be for me to just do that explicit
'unsigned char' even in kernels that don't need it, and also add a
'pr_warn_once()' to make people aware of this case if it ever happens
outside of the xfstests.

So a more complete patch might be something like the attached (which
also changes the polarity of the signed hash test, in order to make
the pr_warn_once() simpler).

            Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1026 bytes --]

 fs/ext4/xattr.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 69a1b8c6a2ec..a2f04a3808db 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -482,11 +482,12 @@ ext4_xattr_inode_verify_hashes(struct inode *ea_inode,
 		 */
 		e_hash = ext4_xattr_hash_entry_signed(entry->e_name, entry->e_name_len,
 							&tmp_data, 1);
-		if (e_hash == entry->e_hash)
-			return 0;
-
 		/* Still no match - bad */
-		return -EFSCORRUPTED;
+		if (e_hash != entry->e_hash)
+			return -EFSCORRUPTED;
+
+		/* Let people know about old hash */
+		pr_warn_once("ext4: filesystem with signed xattr name hash");
 	}
 	return 0;
 }
@@ -3096,7 +3097,7 @@ static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
 	while (name_len--) {
 		hash = (hash << NAME_HASH_SHIFT) ^
 		       (hash >> (8*sizeof(hash) - NAME_HASH_SHIFT)) ^
-		       *name++;
+		       (unsigned char)*name++;
 	}
 	while (value_count--) {
 		hash = (hash << VALUE_HASH_SHIFT) ^

Re: [PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[Theodore Ts'o]

On Tue, Jan 24, 2023 at 09:23:56AM -0800, Linus Torvalds wrote:
> On Tue, Jan 24, 2023 at 8:50 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > This patch does not work correctly without '-funsigned-char', and I
> > don't think that has been back-ported to stable kernels.
> >
> > That said, the patch *almost* works.
> 
> So I'm  not convinced this should be back-ported at all, but it's
> certainly true that going back and forth between the two cases would
> be problematic.

Yeah, I wouldn't backport this patch, since it also requires changes
to e2fsprogs that will take a while to propagate to stable distributions.

   	     	       	      	       		 - Ted

Re: [PATCH AUTOSEL 6.1 35/35] ext4: deal with legacy signed xattr name hash values

[Sasha Levin]

On Wed, Jan 25, 2023 at 11:01:31AM -0500, Theodore Ts'o wrote:
>On Tue, Jan 24, 2023 at 09:23:56AM -0800, Linus Torvalds wrote:
>> On Tue, Jan 24, 2023 at 8:50 AM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>> >
>> > This patch does not work correctly without '-funsigned-char', and I
>> > don't think that has been back-ported to stable kernels.
>> >
>> > That said, the patch *almost* works.
>>
>> So I'm  not convinced this should be back-ported at all, but it's
>> certainly true that going back and forth between the two cases would
>> be problematic.
>
>Yeah, I wouldn't backport this patch, since it also requires changes
>to e2fsprogs that will take a while to propagate to stable distributions.

Ack, I'll drop it. Thanks!

-- 
Thanks,
Sasha