Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [RFC PATCH] ceph: allow encrypting a directory while not having
 Ax caps
To:     =?UTF-8?Q?Lu=c3=ads_Henriques?= <lhenriques@suse.de>
Cc:     Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221027112653.12122-1-lhenriques@suse.de>
 <a992d844-6d75-e134-60e1-acb8c8972ff3@redhat.com> <Y2JZI1QOl3dXBVUb@suse.de>
From:   Xiubo Li <xiubli@redhat.com>
Message-ID: <8e1eecb7-1b54-2459-efc9-2f410ad0a904@redhat.com>
Date:   Thu, 3 Nov 2022 11:18:48 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <Y2JZI1QOl3dXBVUb@suse.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 02/11/2022 19:48, Lu?s Henriques wrote:
> On Mon, Oct 31, 2022 at 05:15:51PM +0800, Xiubo Li wrote:
>> On 27/10/2022 19:26, Lu?s Henriques wrote:
>>> If a client doesn't have Fx caps on a directory, it will get errors while
>>> trying encrypt it:
>>>
>>> ceph: handle_cap_grant: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len 0 new len 48)
>>> fscrypt (ceph, inode 1099511627812): Error -105 getting encryption context
>>>
>>> A simple way to reproduce this is to use two clients:
>>>
>>>       client1 # mkdir /mnt/mydir
>>>
>>>       client2 # ls /mnt/mydir
>>>
>>>       client1 # fscrypt encrypt /mnt/mydir
>>>       client1 # echo hello > /mnt/mydir/world
>>>
>>> This happens because, in __ceph_setattr(), we only initialize
>>> ci->fscrypt_auth if we have Ax.  If we don't have, we'll need to do that
>>> later, in handle_cap_grant().
>>>
>>> Signed-off-by: Lu?s Henriques <lhenriques@suse.de>
>>> ---
>>> Hi!
>>>
>>> To be honest, I'm not really sure about the conditions in the 'if': shall
>>> I bother checking it's really a dir and that it is empty?
>>>
>>> Cheers,
>>> --
>>> Lu?s
>>>
>>>    fs/ceph/caps.c | 26 +++++++++++++++++++++++---
>>>    1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>> index 443fce066d42..e33b5c276cf3 100644
>>> --- a/fs/ceph/caps.c
>>> +++ b/fs/ceph/caps.c
>>> @@ -3511,9 +3511,29 @@ static void handle_cap_grant(struct inode *inode,
>>>    		     from_kuid(&init_user_ns, inode->i_uid),
>>>    		     from_kgid(&init_user_ns, inode->i_gid));
>>>    #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
>>> -		if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
>>> -		    memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
>>> -			   ci->fscrypt_auth_len))
>>> +		if ((ci->fscrypt_auth_len == 0) &&
>>> +		    (extra_info->fscrypt_auth_len > 0) &&
>>> +		    S_ISDIR(inode->i_mode) &&
>>> +		    (ci->i_rsubdirs + ci->i_rfiles == 1)) {
>>> +			/*
>>> +			 * We'll get here when setting up an encrypted directory
>>> +			 * but we don't have Fx in that directory, i.e. other
>>> +			 * clients have accessed this directory too.
>>> +			 */
>>> +			ci->fscrypt_auth = kmemdup(extra_info->fscrypt_auth,
>>> +						   extra_info->fscrypt_auth_len,
>>> +						   GFP_KERNEL);
>>> +			if (ci->fscrypt_auth) {
>>> +				inode->i_flags |= S_ENCRYPTED;
>>> +				ci->fscrypt_auth_len = extra_info->fscrypt_auth_len;
>>> +			} else {
>>> +				pr_err("Failed to alloc memory for %llx.%llx fscrypt_auth\n",
>>> +					ceph_vinop(inode));
>>> +			}
>>> +			dout("ino %llx.%llx is now encrypted\n", ceph_vinop(inode));
>>> +		} else if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
>>> +			   memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
>>> +				  ci->fscrypt_auth_len))
>>>    			pr_warn_ratelimited("%s: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len %d new len %d)\n",
>>>    				__func__, ci->fscrypt_auth_len, extra_info->fscrypt_auth_len);
>>>    #endif
>> Hi Luis,
>>
>> Thanks for your time on this bug.
>>
>> IMO we should fix this in ceph_fill_inode():
>>
>>  ?995 #ifdef CONFIG_FS_ENCRYPTION
>>  ?996???????? if (iinfo->fscrypt_auth_len && (inode->i_state & I_NEW)) {
>>  ?997???????????????? kfree(ci->fscrypt_auth);
>>  ?998???????????????? ci->fscrypt_auth_len = iinfo->fscrypt_auth_len;
>>  ?999???????????????? ci->fscrypt_auth = iinfo->fscrypt_auth;
>> 1000???????????????? iinfo->fscrypt_auth = NULL;
>> 1001???????????????? iinfo->fscrypt_auth_len = 0;
>> 1002???????????????? inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
>> 1003???????? }
>> 1004 #endif
>>
>> The setattr will get a reply from MDS including the fscrypt auth info, I
>> think the kclient just drop it here.
> I've done some testing and I don't really see this code kfree'ing a valid
> fscrypt_auth here.  However, I guess it is possible to fix this issue here
> too, but in a different way, by changing that 'if' condition to:
>
> 	if (iinfo->fscrypt_auth_len &&
> 	    ((inode->i_state & I_NEW) || (ci->fscrypt_auth_len == 0))) {
> 	...
> 	}
>
> I'm not really sure if this is sane though.  When we loose the 'Ax' caps
> (another client as accessed the directory we're encrypting), we also seem
> to loose the I_NEW state.  Using the above code seems to work for the
> testcase in my patch, but I'm not sure it won't break something else.

It should be okay IMO.

The I_NEW is for new created directories, such as for mkdir request,etc. 
But currently the code didn't consider the setattr case.

Please send you patch let's check and discuss there.

Thanks!

- Xiubo

> Cheers,
> --
> Lu?s
>
>> If we fix it in handle_cap_grant() I am afraid this bug still exists. What
>> if there is no any new caps will be issued or revoked recently and then
>> access to the directory ?
>>
>> Thanks
>>
>> - Xiubo
>>