linux-kernel - Re: [PATCH 1/2] fscrypt: new helper function

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]

Date:   Tue, 14 Mar 2023 10:15:11 +0000
From:   Luís Henriques <lhenriques@...e.de>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Xiubo Li <xiubli@...hat.com>, Jeff Layton <jlayton@...nel.org>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Ilya Dryomov <idryomov@...il.com>,
        linux-fscrypt@...r.kernel.org, ceph-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fscrypt: new helper function -
 fscrypt_prepare_atomic_open()

Eric Biggers <ebiggers@...nel.org> writes:

> On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote:
>> + * The regular open path will use fscrypt_file_open for that, but in the
>> + * atomic open a different approach is required.
>
> This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right?

Ups, I missed this comment.

I was comparing the regular open() with the atomic_open() paths.  I think
I really mean fscrypt_file_open() because that's where the encryption info
is (or may be) set by calling fscrypt_require_key().  atomic_open needs
something similar, but combined with a lookup.

Maybe I can rephrase it to:

  The reason for getting the encryption info before checking if the
  directory has the encryption key is because the key may be available but
  the encryption info isn't yet set (maybe due to a drop_caches).  The
  regular open path will call fscrypt_file_open which uses function
  fscrypt_require_key for setting the encryption info if needed.  The
  atomic open needs to do something similar.

Cheers,
-- 
Luís

>> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry)
>> +{
>> +	int err;
>> +
>> +	if (!IS_ENCRYPTED(dir))
>> +		return 0;
>> +
>> +	err = fscrypt_get_encryption_info(dir, true);
>> +	if (!err && !fscrypt_has_encryption_key(dir)) {
>> +		spin_lock(&dentry->d_lock);
>> +		dentry->d_flags |= DCACHE_NOKEY_NAME;
>> +		spin_unlock(&dentry->d_lock);
>> +	}
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open);
> [...]
>> +static inline int fscrypt_prepare_atomic_open(struct inode *dir,
>> +					      struct dentry *dentry)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>
> This has different behavior on unencrypted directories depending on whether
> CONFIG_FS_ENCRYPTION is enabled or not.  That's bad.
>
> In patch 2, the caller you are introducing has already checked IS_ENCRYPTED().
>
> Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for
> *encrypted* directories.
>
> So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION
> version of fscrypt_prepare_atomic_open().
>
> - Eric