diff mbox series

[RFC] ceph: allow encrypting a directory while not having Ax caps

Message ID 20221027112653.12122-1-lhenriques@suse.de
State New
Headers
Series [RFC] ceph: allow encrypting a directory while not having Ax caps |

Commit Message

Luis Henriques Oct. 27, 2022, 11:26 a.m. UTC 
  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(-)

Comments

Xiubo Li Oct. 31, 2022, 9:15 a.m. UTC | #1 
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.

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

>
Luis Henriques Nov. 2, 2022, 11:48 a.m. UTC | #2 
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.

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
> 
> > 
>
Xiubo Li Nov. 3, 2022, 3:18 a.m. UTC | #3 
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
>>
diff mbox series

Patch

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