linux-ext4 - Re: [f2fs-dev] [PATCH 3/7] libfs: Validate negative dentries in case-insensitive directories

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: <874jq10wfy.fsf@suse.de>
Date:   Fri, 31 Mar 2023 12:31:13 -0300
From:   Gabriel Krisman Bertazi <krisman@...e.de>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     kernel@...labora.com, tytso@....edu,
        linux-f2fs-devel@...ts.sourceforge.net, ebiggers@...nel.org,
        linux-fsdevel@...r.kernel.org, jaegeuk@...nel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [f2fs-dev] [PATCH 3/7] libfs: Validate negative dentries in
 case-insensitive directories

Al Viro <viro@...iv.linux.org.uk> writes:

> On Wed, Jun 22, 2022 at 03:45:59PM -0400, Gabriel Krisman Bertazi wrote:
>
>> +static inline int generic_ci_d_revalidate(struct dentry *dentry,
>> +					  const struct qstr *name,
>> +					  unsigned int flags)
>> +{
>> +	int is_creation = flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET);
>> +
>> +	if (d_is_negative(dentry)) {
>> +		const struct dentry *parent = READ_ONCE(dentry->d_parent);
>> +		const struct inode *dir = READ_ONCE(parent->d_inode);
>> +
>> +		if (dir && needs_casefold(dir)) {
>> +			if (!d_is_casefold_lookup(dentry))
>> +				return 0;
>
> 	In which conditions does that happen?

Hi Al,

This can happen right after a case-sensitive directory is converted to
case-insensitive. A previous case-sensitive lookup could have left a
negative dentry in the dcache that we need to reject, because it doesn't
have the same assurance of absence of all-variation of names as a
negative dentry created during a case-insensitive lookup.

>> +			if (is_creation &&
>> +			    (dentry->d_name.len != name->len ||
>> +			     memcmp(dentry->d_name.name, name->name, name->len)))
>> +				return 0;
>> +		}
>> +	}
>> +	return 1;
>> +}
>
> 	Analysis of stability of ->d_name, please.  It's *probably* safe, but
> the details are subtle and IMO should be accompanied by several asserts.
> E.g. "we never get LOOKUP_CREATE in op->intent without O_CREAT in op->open_flag
> for such and such reasons, and we verify that in such and such place"...
>
> 	A part of that would be "the call in lookup_dcache() can only get there
> with non-zero flags when coming from __lookup_hash(), and that has parent locked,
> stabilizing the name; the same goes for the call in __lookup_slow(), with the
> only call chain with possibly non-zero flags is through lookup_slow(), where we
> have the parent locked".  However, lookup_fast() and lookup_open() have the
> flags come from nd->flags, and LOOKUP_CREATE can be found there in several areas.
> I _think_ we are guaranteed the parent locked in all such call chains, but that
> is definitely worth at least a comment.

Thanks for the example of the analysis what you are looking for here.
That will help me quite a bit.  I wrote this code a while ago and I
don't recall the exact details.  I will go through the code again and
send a new version with the detailed analysis.

-- 
Gabriel Krisman Bertazi