linux-kernel - Re: [PATCH v6 01/10] libfs: Create the helper function generic_ci_validate_strict

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a26db27a-85ca-46e4-9669-d885db2dd4ae@igalia.com>
Date: Wed, 16 Oct 2024 18:59:58 -0300
From: André Almeida <andrealmeid@...lia.com>
To: Gabriel Krisman Bertazi <gabriel@...sman.be>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Theodore Ts'o <tytso@....edu>, Andreas Dilger <adilger.kernel@...ger.ca>,
 Hugh Dickins <hughd@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>,
 Jonathan Corbet <corbet@....net>, smcv@...labora.com, kernel-dev@...lia.com,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-ext4@...r.kernel.org, linux-mm@...ck.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v6 01/10] libfs: Create the helper function
 generic_ci_validate_strict_name()

Em 15/10/2024 12:59, Gabriel Krisman Bertazi escreveu:
> André Almeida <andrealmeid@...lia.com> writes:
> 
>> +static inline bool generic_ci_validate_strict_name(struct inode *dir, struct qstr *name)
>> +{
>> +	if (!IS_CASEFOLDED(dir) || !sb_has_strict_encoding(dir->i_sb))
>> +		return true;
>> +
>> +	/*
>> +	 * A casefold dir must have a encoding set, unless the filesystem
>> +	 * is corrupted
>> +	 */
>> +	if (WARN_ON_ONCE(!dir->i_sb->s_encoding))
>> +		return true;
>> +
>> +	return utf8_validate(dir->i_sb->s_encoding, name);
> 
> There is something fishy here.  Concerningly, the fstests test doesn't
> catch it.
> 
> utf8_validate is defined as:
> 
>    int utf8_validate(const struct unicode_map *um, const struct qstr *str)
> 
> Which returns 0 on success and !0 on error. Thus, when casting to bool,
> the return code should be negated.
> 
> But generic/556 doesn't fail. That's because we are over cautious, and
> also check the string at the end of generic_ci_d_hash.  So we never
> really reach utf8_validate in the tested case.
> 
> But if you comment the final if in generic_ci_d_hash, you'll see this
> patchset regresses the fstests case generic/556 over ext4.
> 
> We really need the check in both places, though.  We don't want to rely
> on the behavior of generic_ci_d_hash to block invalid filenames, as that
> might change.
> 

Thanks Krisman! Nice catch. I fixed this for the next version. Testing 
with the modified generic_ci_d_hash(), I also realized that the 
validation was in the wrong place and leaving an inode behind, this fixed:

diff --git a/mm/shmem.c b/mm/shmem.c
index eb1ea1f3b37c..7bd7ca5777af 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3624,13 +3624,13 @@ shmem_mknod(struct mnt_idmap *idmap, struct 
inode *dir,
         struct inode *inode;
         int error;

+       if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
+               return -EINVAL;
+
         inode = shmem_get_inode(idmap, dir->i_sb, dir, mode, dev, 
VM_NORESERVE);
         if (IS_ERR(inode))
                 return PTR_ERR(inode);

-       if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
-               return -EINVAL;
-