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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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
>
> >
>
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 <[email protected]>
>>> ---
>>> 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
>>