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
| ||
|
Message-ID: <87a5otxj47.fsf@mailhost.krisman.be> Date: Thu, 25 Jan 2024 17:18:00 -0300 From: Gabriel Krisman Bertazi <krisman@...e.de> To: Eric Biggers <ebiggers@...nel.org> Cc: viro@...iv.linux.org.uk, jaegeuk@...nel.org, tytso@....edu, linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net, linux-fsdevel@...r.kernel.org, amir73il@...il.com Subject: Re: [PATCH v3 02/10] fscrypt: Share code between functions that prepare lookup Eric Biggers <ebiggers@...nel.org> writes: > On Fri, Jan 19, 2024 at 03:47:34PM -0300, Gabriel Krisman Bertazi wrote: >> To make the patch simpler, we now call fscrypt_get_encryption_info twice >> for fscrypt_prepare_lookup, once inside fscrypt_setup_filename and once >> inside fscrypt_prepare_lookup_dentry. It seems safe to do, and >> considering it will bail early in the second lookup and most lookups >> should go to the dcache anyway, it doesn't seem problematic for >> performance. In addition, we add a function call for the unencrypted >> case, also during lookup. > > Unfortunately I don't think it's correct. This is basically undoing my fix > b01531db6cec ("fscrypt: fix race where ->lookup() marks plaintext dentry as > ciphertext") from several years ago. > > When a lookup is done, the filesystem needs to either treat the name being > looked up as a no-key name *or* as a regular name, depending on whether the > directory's key is present. We shouldn't enable race conditions where, due to > the key being concurrently added, the name is treated as a no-key name for > filename matching purposes but a regular name for dentry validation purposes. > That can result in an anomaly where a file that exists ends up with a negative > dentry that doesn't get invalidated. > > Basically, the boolean fscrypt_name::is_nokey_name that's produced by > fscrypt_setup_filename() should continue to be propagated to DCACHE_NOKEY_NAME. I see your point. I'll drop this patch and replace it with a patch that just merges the DCACHE_NOKEY_NAME configuration. Sadly, we gotta keep the two variants I think. thanks for the review -- Gabriel Krisman Bertazi
Powered by blists - more mailing lists