linux-kernel - Re: [PATCH v2] ceph: make sure directories aren't complete after setting crypt context

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: <fbce8a0b-340b-0d82-ffbc-1245e30876f9@redhat.com>
Date:   Tue, 22 Nov 2022 09:02:15 +0800
From:   Xiubo Li <xiubli@...hat.com>
To:     Luís Henriques <lhenriques@...e.de>,
        Ilya Dryomov <idryomov@...il.com>,
        Jeff Layton <jlayton@...nel.org>
Cc:     ceph-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ceph: make sure directories aren't complete after
 setting crypt context


On 22/11/2022 02:00, Luís Henriques wrote:
> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
> to be used otherwise, if it was complete before, any old dentry that's still
> around will be valid.
>
> Signed-off-by: Luís Henriques <lhenriques@...e.de>
> ---
> Hi Xiubo!
>
> I've added the __fscrypt_prepare_readdir() wrapper as you suggested, but I
> had to change it slightly because we also need to handle the error cases.
>
> Changes since v1:
> - Moved the __ceph_dir_clear_complete() call from ceph_crypt_get_context()
>    to ceph_lookup().
> - Added an __fscrypt_prepare_readdir() wrapper to check key status changes
>
>   fs/ceph/dir.c | 31 ++++++++++++++++++++++++++++---
>   1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index edc2bf0aab83..2cac7e3ab352 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -763,6 +763,27 @@ static bool is_root_ceph_dentry(struct inode *inode, struct dentry *dentry)
>   		strncmp(dentry->d_name.name, ".ceph", 5) == 0;
>   }
>   
> +/*
> + * Simple wrapper around __fscrypt_prepare_readdir() that will return:
> + *
> + * - '1' if directory was locked and key is now loaded (i.e. dir is unlocked),
> + * - '0' if directory is still locked, or
> + * - an error (< 0) if __fscrypt_prepare_readdir() fails.
> + */
> +static int ceph_fscrypt_prepare_readdir(struct inode *dir)
> +{
> +	bool had_key = fscrypt_has_encryption_key(dir);
> +	int err;
> +
> +	err = __fscrypt_prepare_readdir(dir);
> +	if (err)
> +		return err;
> +	/* is directory now unlocked? */
> +	if (!had_key && fscrypt_has_encryption_key(dir))
> +		return 1;
> +	return 0;
> +}
> +
>   /*
>    * Look up a single dir entry.  If there is a lookup intent, inform
>    * the MDS so that it gets our 'caps wanted' value in a single op.
> @@ -784,10 +805,14 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>   		return ERR_PTR(-ENAMETOOLONG);
>   
>   	if (IS_ENCRYPTED(dir)) {
> -		err = __fscrypt_prepare_readdir(dir);
> -		if (err)
> +		err = ceph_fscrypt_prepare_readdir(dir);
> +		if (err < 0)
>   			return ERR_PTR(err);
> -		if (!fscrypt_has_encryption_key(dir)) {
> +		if (err) {
> +			/* directory just got unlocked */
> +			__ceph_dir_clear_complete(ceph_inode(dir));

Luis,

Could we just move this into ceph_fscrypt_prepare_readdir() ? IMO we 
should always clear the complete flag always whenever the key is first 
loaded.

> +		} else {
> +			/* no encryption key */

I think you also need to fix all the other places, which are calling the 
__fscrypt_prepare_readdir() or fscrypt_prepare_readdir() in ceph. 
Because we don't know the ceph_lookup() will always be the first caller 
to trigger __fscrypt_prepare_readdir() for a dir.

Thanks!

- Xiubo

>   			spin_lock(&dentry->d_lock);
>   			dentry->d_flags |= DCACHE_NOKEY_NAME;
>   			spin_unlock(&dentry->d_lock);
>