linux-ext4 - Re: [PATCH v5 06/10] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230814184214.GB1171@sol.localdomain>
Date:   Mon, 14 Aug 2023 11:42:14 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Gabriel Krisman Bertazi <krisman@...e.de>
Cc:     viro@...iv.linux.org.uk, brauner@...nel.org, tytso@....edu,
        jaegeuk@...nel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        Gabriel Krisman Bertazi <krisman@...labora.com>
Subject: Re: [PATCH v5 06/10] libfs: Validate negative dentries in
 case-insensitive directories

On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@...nel.org> writes:
> 
> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> >> +	/*
> >> +	 * Filesystems will call into d_revalidate without setting
> >> +	 * LOOKUP_ flags even for file creation (see lookup_one*
> >> +	 * variants).  Reject negative dentries in this case, since we
> >> +	 * can't know for sure it won't be used for creation.
> >> +	 */
> >> +	if (!flags)
> >> +		return 0;
> >> +
> >> +	/*
> >> +	 * If the lookup is for creation, then a negative dentry can
> >> +	 * only be reused if it's a case-sensitive match, not just a
> >> +	 * case-insensitive one.  This is needed to make the new file be
> >> +	 * created with the name the user specified, preserving case.
> >> +	 */
> >> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> +		/*
> >> +		 * ->d_name won't change from under us in the creation
> >> +		 * path only, since d_revalidate during creation and
> >> +		 * renames is always called with the parent inode
> >> +		 * locked.  It isn't the case for all lookup callpaths,
> >> +		 * so ->d_name must not be touched outside
> >> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> >> +		 */
> >> +		if (dentry->d_name.len != name->len ||
> >> +		    memcmp(dentry->d_name.name, name->name, name->len))
> >> +			return 0;
> >> +	}
> >
> > This is still really confusing to me.  Can you consider the below?  The code is
> > the same except for the reordering, but the explanation is reworked to be much
> > clearer (IMO).  Anything I am misunderstanding?
> >
> > 	/*
> > 	 * If the lookup is for creation, then a negative dentry can only be
> > 	 * reused if it's a case-sensitive match, not just a case-insensitive
> > 	 * one.  This is needed to make the new file be created with the name
> > 	 * the user specified, preserving case.
> > 	 *
> > 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
> > 	 * cases, ->d_name is stable and can be compared to 'name' without
> > 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
> > 	 * (This is because the directory lock blocks the dentry from being
> > 	 * concurrently instantiated, and negative dentries are never moved.)
> > 	 *
> > 	 * All other creations actually use flags==0.  These come from the edge
> > 	 * case of filesystems calling functions like lookup_one() that do a
> > 	 * lookup without setting the lookup flags at all.  Such lookups might
> > 	 * or might not be for creation, and if not don't guarantee stable
> > 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
> > 	 */
> > 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> > 		if (dentry->d_name.len != name->len ||
> > 		    memcmp(dentry->d_name.name, name->name, name->len))
> > 			return 0;
> > 	}
> > 	if (!flags)
> > 		return 0;
> 
> I don't see it as particularly better or less confusing than the
> original. but I also don't mind taking it into the next iteration.
> 

Your commit message is still much longer and covers some quite different details
which seem irrelevant to me.  So if you don't see my explanation as being much
different, I think we're still not on the same page.  I hope that I'm not
misunderstanding anything, in which I believe that what I wrote above is a good
explanation and your commit message should be substantially simplified.
Remember, longer != better.  Keep things as simple as possible.

- Eric