[PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Luís Henriques]

Hi!

I'm sending another iteration of the encrypted snapshot names patch.  This
patch assumes PR#45224 [1] to be merged as it adds support for the
alternate names.

Two notes:

1. Patch 0001 is just a small fix from another fscrypt patch.  It's
   probably better to simply squash it.

2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
   reproduce it easily by commenting the code that adds the
   DCACHE_NOKEY_NAME flag in patch 0003.

Any comments are welcome (including for the PR mentioned above, of course).

[1] https://github.com/ceph/ceph/pull/45224

Luís Henriques (3):
  ceph: fix error path in ceph_readdir()
  ceph: fix use-after-free in ceph_readdir
  ceph: add support for encrypted snapshot names

 fs/ceph/dir.c   | 11 ++++++++++-
 fs/ceph/inode.c | 13 +++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

[PATCH 1/3] ceph: fix error path in ceph_readdir()

[Luís Henriques]

This was introduced by commit

  "ceph: add support to readdir for encrypted filenames"

It will eventually leak the fscrypt_str names in this error path.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/dir.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 44395aae7259..0bcb677d2199 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -500,7 +500,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 					       next_offset);
 			if (err) {
 				ceph_mdsc_put_request(dfi->last_readdir);
-				return err;
 				goto out;
 			}
 		} else if (req->r_reply_info.dir_end) {

[PATCH 2/3] ceph: fix use-after-free in ceph_readdir

[Luís Henriques]

After calling ceph_mdsc_put_request() on dfi->last_readdir, this field
should be set to NULL, otherwise we may end-up freeing it twince and get
the following splat:

  refcount_t: underflow; use-after-free.
  WARNING: CPU: 0 PID: 229 at lib/refcount.c:28 refcount_warn_saturate+0xa6/0xf0
  ...
  Call Trace:
    <TASK>
    ceph_readdir+0xd35/0x1460 [ceph]
    ? _raw_spin_unlock+0x12/0x30
    ? preempt_count_add+0x73/0xa0
    ? _raw_spin_unlock+0x12/0x30
    ? __mark_inode_dirty+0x27c/0x3a0
    iterate_dir+0x7d/0x190
    __x64_sys_getdents64+0x80/0x120
    ? compat_fillonedir+0x160/0x160
    do_syscall_64+0x43/0x90
    entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 0bcb677d2199..934402f5e9e6 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -555,6 +555,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
 			dout("filldir stopping us...\n");
 			ceph_mdsc_put_request(dfi->last_readdir);
+			dfi->last_readdir = NULL;
 			err = 0;
 			goto out;
 		}

[PATCH 3/3] ceph: add support for encrypted snapshot names

[Luís Henriques]

Since filenames in encrypted directories are already encrypted and shown
as a base64-encoded string when the directory is locked, snapshot names
should show a similar behaviour.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/dir.c   |  9 +++++++++
 fs/ceph/inode.c | 13 +++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 934402f5e9e6..17d2f18a1fd1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1069,6 +1069,15 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 		op = CEPH_MDS_OP_MKSNAP;
 		dout("mksnap dir %p snap '%pd' dn %p\n", dir,
 		     dentry, dentry);
+		/*
+		 * Encrypted snapshots require d_revalidate to force a
+		 * LOOKUPSNAP to cleanup dcache
+		 */
+		if (IS_ENCRYPTED(dir)) {
+			spin_lock(&dentry->d_lock);
+			dentry->d_flags |= DCACHE_NOKEY_NAME;
+			spin_unlock(&dentry->d_lock);
+		}
 	} else if (ceph_snap(dir) == CEPH_NOSNAP) {
 		dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
 		op = CEPH_MDS_OP_MKDIR;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8b0832271fdf..357335a11384 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	ci->i_rbytes = 0;
 	ci->i_btime = ceph_inode(parent)->i_btime;
 
+	/* if encrypted, just borrow fscrypt_auth from parent */
+	if (IS_ENCRYPTED(parent)) {
+		struct ceph_inode_info *pci = ceph_inode(parent);
+
+		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
+					   pci->fscrypt_auth_len,
+					   GFP_KERNEL);
+		if (ci->fscrypt_auth) {
+			inode->i_flags |= S_ENCRYPTED;
+			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
+		} else
+			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
+	}
 	if (inode->i_state & I_NEW) {
 		inode->i_op = &ceph_snapdir_iops;
 		inode->i_fop = &ceph_snapdir_fops;

Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Luís Henriques]

Luís Henriques <lhenriques@suse.de> writes:

> Hi!
>
> I'm sending another iteration of the encrypted snapshot names patch.  This
> patch assumes PR#45224 [1] to be merged as it adds support for the
> alternate names.
>
> Two notes:
>
> 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
>    probably better to simply squash it.
>
> 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
>    reproduce it easily by commenting the code that adds the
>    DCACHE_NOKEY_NAME flag in patch 0003.

Obviously, immediately after sending this patchset I realized I failed to
mention a very (*VERY*) important note:

Snapshot names can not start with a '_'.  I think the reason is related
with the 'long snapshot names', but I can't really remember the details
anymore.  The point is that an encrypted snapshot name base64-encoded
*may* end-up starting with an '_' as we're using the base64-url variant.

I really don't know if it's possible to fix that.  I guess that in that
case the user will get an error and fail to create the snapshot but he'll
be clueless because the reason.  Probably a warning can be added to the
kernel logs, but maybe there are other ideas.

Cheers,
-- 
Luís


> Any comments are welcome (including for the PR mentioned above, of course).
>
> [1] https://github.com/ceph/ceph/pull/45224
>
> Luís Henriques (3):
>   ceph: fix error path in ceph_readdir()
>   ceph: fix use-after-free in ceph_readdir
>   ceph: add support for encrypted snapshot names
>
>  fs/ceph/dir.c   | 11 ++++++++++-
>  fs/ceph/inode.c | 13 +++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>

Re: [PATCH 1/3] ceph: fix error path in ceph_readdir()

[Jeff Layton]

On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
> This was introduced by commit
> 
>   "ceph: add support to readdir for encrypted filenames"
> 
> It will eventually leak the fscrypt_str names in this error path.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/dir.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 44395aae7259..0bcb677d2199 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -500,7 +500,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  					       next_offset);
>  			if (err) {
>  				ceph_mdsc_put_request(dfi->last_readdir);
> -				return err;
>  				goto out;
>  			}
>  		} else if (req->r_reply_info.dir_end) {

This doesn't seem to apply to the current wip-fscrypt tree. Are you
working against an older branch, perhaps?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] ceph: fix use-after-free in ceph_readdir

[Jeff Layton]

On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
> After calling ceph_mdsc_put_request() on dfi->last_readdir, this field
> should be set to NULL, otherwise we may end-up freeing it twince and get
> the following splat:
> 
>   refcount_t: underflow; use-after-free.
>   WARNING: CPU: 0 PID: 229 at lib/refcount.c:28 refcount_warn_saturate+0xa6/0xf0
>   ...
>   Call Trace:
>     <TASK>
>     ceph_readdir+0xd35/0x1460 [ceph]
>     ? _raw_spin_unlock+0x12/0x30
>     ? preempt_count_add+0x73/0xa0
>     ? _raw_spin_unlock+0x12/0x30
>     ? __mark_inode_dirty+0x27c/0x3a0
>     iterate_dir+0x7d/0x190
>     __x64_sys_getdents64+0x80/0x120
>     ? compat_fillonedir+0x160/0x160
>     do_syscall_64+0x43/0x90
>     entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/dir.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 0bcb677d2199..934402f5e9e6 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -555,6 +555,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>  			dout("filldir stopping us...\n");
>  			ceph_mdsc_put_request(dfi->last_readdir);
> +			dfi->last_readdir = NULL;
>  			err = 0;
>  			goto out;
>  		}

I think Xiubo fixed this in the testing branch late yesterday. It should
no longer be needed.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/3] ceph: add support for encrypted snapshot names

[Jeff Layton]

On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
> Since filenames in encrypted directories are already encrypted and shown
> as a base64-encoded string when the directory is locked, snapshot names
> should show a similar behaviour.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  fs/ceph/dir.c   |  9 +++++++++
>  fs/ceph/inode.c | 13 +++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 934402f5e9e6..17d2f18a1fd1 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1069,6 +1069,15 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>  		op = CEPH_MDS_OP_MKSNAP;
>  		dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>  		     dentry, dentry);
> +		/*
> +		 * Encrypted snapshots require d_revalidate to force a
> +		 * LOOKUPSNAP to cleanup dcache
> +		 */
> +		if (IS_ENCRYPTED(dir)) {
> +			spin_lock(&dentry->d_lock);
> +			dentry->d_flags |= DCACHE_NOKEY_NAME;
> +			spin_unlock(&dentry->d_lock);
> +		}
>  	} else if (ceph_snap(dir) == CEPH_NOSNAP) {
>  		dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>  		op = CEPH_MDS_OP_MKDIR;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..357335a11384 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>  	ci->i_rbytes = 0;
>  	ci->i_btime = ceph_inode(parent)->i_btime;
>  
> +	/* if encrypted, just borrow fscrypt_auth from parent */
> +	if (IS_ENCRYPTED(parent)) {
> +		struct ceph_inode_info *pci = ceph_inode(parent);
> +
> +		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
> +					   pci->fscrypt_auth_len,
> +					   GFP_KERNEL);
> +		if (ci->fscrypt_auth) {
> +			inode->i_flags |= S_ENCRYPTED;
> +			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> +		} else
> +			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");

Should we return an error in this case?

> +	}
>  	if (inode->i_state & I_NEW) {
>  		inode->i_op = &ceph_snapdir_iops;
>  		inode->i_fop = &ceph_snapdir_fops;

Seems simple and straightforward at first glance.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Jeff Layton]

On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
> Luís Henriques <lhenriques@suse.de> writes:
> 
> > Hi!
> > 
> > I'm sending another iteration of the encrypted snapshot names patch.  This
> > patch assumes PR#45224 [1] to be merged as it adds support for the
> > alternate names.
> > 
> > Two notes:
> > 
> > 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
> >    probably better to simply squash it.
> > 
> > 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
> >    reproduce it easily by commenting the code that adds the
> >    DCACHE_NOKEY_NAME flag in patch 0003.
> 
> Obviously, immediately after sending this patchset I realized I failed to
> mention a very (*VERY*) important note:
> 
> Snapshot names can not start with a '_'.  I think the reason is related
> with the 'long snapshot names', but I can't really remember the details
> anymore.  The point is that an encrypted snapshot name base64-encoded
> *may* end-up starting with an '_' as we're using the base64-url variant.
> 
> I really don't know if it's possible to fix that.  I guess that in that
> case the user will get an error and fail to create the snapshot but he'll
> be clueless because the reason.  Probably a warning can be added to the
> kernel logs, but maybe there are other ideas.
> 


Ouch. Is that imposed by the MDS? It'd be best if we could remove that
limitation from it altogether if we can.

If we can't, then we might be able to get away with prepending all the
encrypted names with some legal characte. Then when we go to decrypt it
we just strip that off.

We could also consider changing the base64 routine to use something else
in lieu of '_' but that's more of a hassle.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] ceph: fix use-after-free in ceph_readdir

[Xiubo Li]

On 3/5/22 2:20 AM, Jeff Layton wrote:
> On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
>> After calling ceph_mdsc_put_request() on dfi->last_readdir, this field
>> should be set to NULL, otherwise we may end-up freeing it twince and get
>> the following splat:
>>
>>    refcount_t: underflow; use-after-free.
>>    WARNING: CPU: 0 PID: 229 at lib/refcount.c:28 refcount_warn_saturate+0xa6/0xf0
>>    ...
>>    Call Trace:
>>      <TASK>
>>      ceph_readdir+0xd35/0x1460 [ceph]
>>      ? _raw_spin_unlock+0x12/0x30
>>      ? preempt_count_add+0x73/0xa0
>>      ? _raw_spin_unlock+0x12/0x30
>>      ? __mark_inode_dirty+0x27c/0x3a0
>>      iterate_dir+0x7d/0x190
>>      __x64_sys_getdents64+0x80/0x120
>>      ? compat_fillonedir+0x160/0x160
>>      do_syscall_64+0x43/0x90
>>      entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/dir.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 0bcb677d2199..934402f5e9e6 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -555,6 +555,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>   			dout("filldir stopping us...\n");
>>   			ceph_mdsc_put_request(dfi->last_readdir);
>> +			dfi->last_readdir = NULL;
>>   			err = 0;
>>   			goto out;
>>   		}
> I think Xiubo fixed this in the testing branch late yesterday. It should
> no longer be needed.

Right and I have sent a new version of my previous patch to remove the 
buggy code.

- Xiubo

> Thanks,

Re: [PATCH 3/3] ceph: add support for encrypted snapshot names

[Xiubo Li]

On 3/5/22 12:14 AM, Luís Henriques wrote:
> Since filenames in encrypted directories are already encrypted and shown
> as a base64-encoded string when the directory is locked, snapshot names
> should show a similar behaviour.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/dir.c   |  9 +++++++++
>   fs/ceph/inode.c | 13 +++++++++++++
>   2 files changed, 22 insertions(+)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 934402f5e9e6..17d2f18a1fd1 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1069,6 +1069,15 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>   		op = CEPH_MDS_OP_MKSNAP;
>   		dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>   		     dentry, dentry);
> +		/*
> +		 * Encrypted snapshots require d_revalidate to force a
> +		 * LOOKUPSNAP to cleanup dcache
> +		 */
> +		if (IS_ENCRYPTED(dir)) {
> +			spin_lock(&dentry->d_lock);
> +			dentry->d_flags |= DCACHE_NOKEY_NAME;
> +			spin_unlock(&dentry->d_lock);
> +		}

Yeah, this fix looks cool.


>   	} else if (ceph_snap(dir) == CEPH_NOSNAP) {
>   		dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>   		op = CEPH_MDS_OP_MKDIR;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..357335a11384 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>   	ci->i_rbytes = 0;
>   	ci->i_btime = ceph_inode(parent)->i_btime;
>   
> +	/* if encrypted, just borrow fscrypt_auth from parent */
> +	if (IS_ENCRYPTED(parent)) {
> +		struct ceph_inode_info *pci = ceph_inode(parent);
> +
> +		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
> +					   pci->fscrypt_auth_len,
> +					   GFP_KERNEL);
> +		if (ci->fscrypt_auth) {
> +			inode->i_flags |= S_ENCRYPTED;
> +			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> +		} else
> +			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
> +	}
>   	if (inode->i_state & I_NEW) {
>   		inode->i_op = &ceph_snapdir_iops;
>   		inode->i_fop = &ceph_snapdir_fops;
>

Re: [PATCH 1/3] ceph: fix error path in ceph_readdir()

[Luís Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
>> This was introduced by commit
>> 
>>   "ceph: add support to readdir for encrypted filenames"
>> 
>> It will eventually leak the fscrypt_str names in this error path.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>  fs/ceph/dir.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 44395aae7259..0bcb677d2199 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -500,7 +500,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>  					       next_offset);
>>  			if (err) {
>>  				ceph_mdsc_put_request(dfi->last_readdir);
>> -				return err;
>>  				goto out;
>>  			}
>>  		} else if (req->r_reply_info.dir_end) {
>
> This doesn't seem to apply to the current wip-fscrypt tree. Are you
> working against an older branch, perhaps?

I thought I was on an up-to-date branch but obviously I was wrong.  I'll
rebase the patches.  Sorry.

Cheers,
-- 
Luís

>
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/3] ceph: fix use-after-free in ceph_readdir

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/5/22 2:20 AM, Jeff Layton wrote:
>> On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
>>> After calling ceph_mdsc_put_request() on dfi->last_readdir, this field
>>> should be set to NULL, otherwise we may end-up freeing it twince and get
>>> the following splat:
>>>
>>>    refcount_t: underflow; use-after-free.
>>>    WARNING: CPU: 0 PID: 229 at lib/refcount.c:28 refcount_warn_saturate+0xa6/0xf0
>>>    ...
>>>    Call Trace:
>>>      <TASK>
>>>      ceph_readdir+0xd35/0x1460 [ceph]
>>>      ? _raw_spin_unlock+0x12/0x30
>>>      ? preempt_count_add+0x73/0xa0
>>>      ? _raw_spin_unlock+0x12/0x30
>>>      ? __mark_inode_dirty+0x27c/0x3a0
>>>      iterate_dir+0x7d/0x190
>>>      __x64_sys_getdents64+0x80/0x120
>>>      ? compat_fillonedir+0x160/0x160
>>>      do_syscall_64+0x43/0x90
>>>      entry_SYSCALL_64_after_hwframe+0x44/0xae
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>   fs/ceph/dir.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index 0bcb677d2199..934402f5e9e6 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -555,6 +555,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>   			dout("filldir stopping us...\n");
>>>   			ceph_mdsc_put_request(dfi->last_readdir);
>>> +			dfi->last_readdir = NULL;
>>>   			err = 0;
>>>   			goto out;
>>>   		}
>> I think Xiubo fixed this in the testing branch late yesterday. It should
>> no longer be needed.
>
> Right and I have sent a new version of my previous patch to remove the buggy
> code.

Ok, cool.  This definitely proofs that my local branch wasn't updated :-)

(I really need to get rid of this mails/patches backlog.)

Cheers,
-- 
Luís

Re: [PATCH 3/3] ceph: add support for encrypted snapshot names

[Luís Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2022-03-04 at 16:14 +0000, Luís Henriques wrote:
>> Since filenames in encrypted directories are already encrypted and shown
>> as a base64-encoded string when the directory is locked, snapshot names
>> should show a similar behaviour.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>  fs/ceph/dir.c   |  9 +++++++++
>>  fs/ceph/inode.c | 13 +++++++++++++
>>  2 files changed, 22 insertions(+)
>> 
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 934402f5e9e6..17d2f18a1fd1 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -1069,6 +1069,15 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>  		op = CEPH_MDS_OP_MKSNAP;
>>  		dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>  		     dentry, dentry);
>> +		/*
>> +		 * Encrypted snapshots require d_revalidate to force a
>> +		 * LOOKUPSNAP to cleanup dcache
>> +		 */
>> +		if (IS_ENCRYPTED(dir)) {
>> +			spin_lock(&dentry->d_lock);
>> +			dentry->d_flags |= DCACHE_NOKEY_NAME;
>> +			spin_unlock(&dentry->d_lock);
>> +		}
>>  	} else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>  		dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>  		op = CEPH_MDS_OP_MKDIR;
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 8b0832271fdf..357335a11384 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>>  	ci->i_rbytes = 0;
>>  	ci->i_btime = ceph_inode(parent)->i_btime;
>>  
>> +	/* if encrypted, just borrow fscrypt_auth from parent */
>> +	if (IS_ENCRYPTED(parent)) {
>> +		struct ceph_inode_info *pci = ceph_inode(parent);
>> +
>> +		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>> +					   pci->fscrypt_auth_len,
>> +					   GFP_KERNEL);
>> +		if (ci->fscrypt_auth) {
>> +			inode->i_flags |= S_ENCRYPTED;
>> +			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> +		} else
>> +			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
>
> Should we return an error in this case?

Yeah, definitely.  I'll add that and send out v2.  Thanks.

Cheers,
-- 
Luís


>
>> +	}
>>  	if (inode->i_state & I_NEW) {
>>  		inode->i_op = &ceph_snapdir_iops;
>>  		inode->i_fop = &ceph_snapdir_fops;
>
> Seems simple and straightforward at first glance.
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Luís Henriques]

Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
>> Luís Henriques <lhenriques@suse.de> writes:
>> 
>> > Hi!
>> > 
>> > I'm sending another iteration of the encrypted snapshot names patch.  This
>> > patch assumes PR#45224 [1] to be merged as it adds support for the
>> > alternate names.
>> > 
>> > Two notes:
>> > 
>> > 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
>> >    probably better to simply squash it.
>> > 
>> > 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
>> >    reproduce it easily by commenting the code that adds the
>> >    DCACHE_NOKEY_NAME flag in patch 0003.
>> 
>> Obviously, immediately after sending this patchset I realized I failed to
>> mention a very (*VERY*) important note:
>> 
>> Snapshot names can not start with a '_'.  I think the reason is related
>> with the 'long snapshot names', but I can't really remember the details
>> anymore.  The point is that an encrypted snapshot name base64-encoded
>> *may* end-up starting with an '_' as we're using the base64-url variant.
>> 
>> I really don't know if it's possible to fix that.  I guess that in that
>> case the user will get an error and fail to create the snapshot but he'll
>> be clueless because the reason.  Probably a warning can be added to the
>> kernel logs, but maybe there are other ideas.
>> 
>
>
> Ouch. Is that imposed by the MDS? It'd be best if we could remove that
> limitation from it altogether if we can.

I do remember hitting this limitation in the past, but a quick grep didn't
show anything in the documentation about it.  This seems to have been
added to the MDS a *long* time ago, with commit 068553473c82 ("mds: adjust
trace encoding, clean up snap naming") but (as usual) there aren't a lot
of details.

>
> If we can't, then we might be able to get away with prepending all the
> encrypted names with some legal characte. Then when we go to decrypt it
> we just strip that off.

This is probably the best way to fix it, but it's worth trying to find
out the origins of this limitation.  I do seem to remember some obscure
reasons, related with the long snap names (for which Xiubo has a patch),
which will start with '_'.  But yeah I'll have to go dig deeper.

> We could also consider changing the base64 routine to use something else
> in lieu of '_' but that's more of a hassle.

Cheers,
-- 
Luís

Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Xiubo Li]

On 3/5/22 10:56 PM, Luís Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
>
>> On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
>>> Luís Henriques <lhenriques@suse.de> writes:
>>>
>>>> Hi!
>>>>
>>>> I'm sending another iteration of the encrypted snapshot names patch.  This
>>>> patch assumes PR#45224 [1] to be merged as it adds support for the
>>>> alternate names.
>>>>
>>>> Two notes:
>>>>
>>>> 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
>>>>     probably better to simply squash it.
>>>>
>>>> 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
>>>>     reproduce it easily by commenting the code that adds the
>>>>     DCACHE_NOKEY_NAME flag in patch 0003.
>>> Obviously, immediately after sending this patchset I realized I failed to
>>> mention a very (*VERY*) important note:
>>>
>>> Snapshot names can not start with a '_'.  I think the reason is related
>>> with the 'long snapshot names', but I can't really remember the details
>>> anymore.  The point is that an encrypted snapshot name base64-encoded
>>> *may* end-up starting with an '_' as we're using the base64-url variant.
>>>
>>> I really don't know if it's possible to fix that.  I guess that in that
>>> case the user will get an error and fail to create the snapshot but he'll
>>> be clueless because the reason.  Probably a warning can be added to the
>>> kernel logs, but maybe there are other ideas.
>>>
>>
>> Ouch. Is that imposed by the MDS? It'd be best if we could remove that
>> limitation from it altogether if we can.
> I do remember hitting this limitation in the past, but a quick grep didn't
> show anything in the documentation about it.  This seems to have been
> added to the MDS a *long* time ago, with commit 068553473c82 ("mds: adjust
> trace encoding, clean up snap naming") but (as usual) there aren't a lot
> of details.

When making a snapshot and in MDS code:

10458   if (snapname.length() == 0 ||
10459       snapname[0] == '_') {
10460     respond_to_request(mdr, -CEPHFS_EINVAL);
10461     return;
10462   }


>> If we can't, then we might be able to get away with prepending all the
>> encrypted names with some legal characte. Then when we go to decrypt it
>> we just strip that off.
> This is probably the best way to fix it, but it's worth trying to find
> out the origins of this limitation.  I do seem to remember some obscure
> reasons, related with the long snap names (for which Xiubo has a patch),
> which will start with '_'.  But yeah I'll have to go dig deeper.

It will recognize the encrypted "_XYZ_${DIGIT}" snapshot name as the 
long snapshot name inherited from its parent snap realm, and will parse 
the "${DIGIT}" as an ino in other places.

Maybe in MDS we should fail the request only when snapshot name is in 
type of "_XYZ_${DIGIT}" instead of only "_XYZ", and in client side 
should also print one error or warn log about this ?

This why added the ceph PR[1] to tell the kclient current snapshot name 
is a long snap name in lssnap. So if we can forbid the snap shot name 
begin with '_' it will simple in kclient code to handle the long snap 
name, or it will be complex in both MDS and kclient.

[1] https://github.com/ceph/ceph/pull/45208

-- Xiubo


>> We could also consider changing the base64 routine to use something else
>> in lieu of '_' but that's more of a hassle.
> Cheers,

Re: [PATCH 0/3] ceph: minor fixes and encrypted snapshot names

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/5/22 10:56 PM, Luís Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>>
>>> On Fri, 2022-03-04 at 16:26 +0000, Luís Henriques wrote:
>>>> Luís Henriques <lhenriques@suse.de> writes:
>>>>
>>>>> Hi!
>>>>>
>>>>> I'm sending another iteration of the encrypted snapshot names patch.  This
>>>>> patch assumes PR#45224 [1] to be merged as it adds support for the
>>>>> alternate names.
>>>>>
>>>>> Two notes:
>>>>>
>>>>> 1. Patch 0001 is just a small fix from another fscrypt patch.  It's
>>>>>     probably better to simply squash it.
>>>>>
>>>>> 2. I'm not sure how easy it is to hit the UAF fixed by patch 0002.  I can
>>>>>     reproduce it easily by commenting the code that adds the
>>>>>     DCACHE_NOKEY_NAME flag in patch 0003.
>>>> Obviously, immediately after sending this patchset I realized I failed to
>>>> mention a very (*VERY*) important note:
>>>>
>>>> Snapshot names can not start with a '_'.  I think the reason is related
>>>> with the 'long snapshot names', but I can't really remember the details
>>>> anymore.  The point is that an encrypted snapshot name base64-encoded
>>>> *may* end-up starting with an '_' as we're using the base64-url variant.
>>>>
>>>> I really don't know if it's possible to fix that.  I guess that in that
>>>> case the user will get an error and fail to create the snapshot but he'll
>>>> be clueless because the reason.  Probably a warning can be added to the
>>>> kernel logs, but maybe there are other ideas.
>>>>
>>>
>>> Ouch. Is that imposed by the MDS? It'd be best if we could remove that
>>> limitation from it altogether if we can.
>> I do remember hitting this limitation in the past, but a quick grep didn't
>> show anything in the documentation about it.  This seems to have been
>> added to the MDS a *long* time ago, with commit 068553473c82 ("mds: adjust
>> trace encoding, clean up snap naming") but (as usual) there aren't a lot
>> of details.
>
> When making a snapshot and in MDS code:
>
> 10458   if (snapname.length() == 0 ||
> 10459       snapname[0] == '_') {
> 10460     respond_to_request(mdr, -CEPHFS_EINVAL);
> 10461     return;
> 10462   }
>
>
>>> If we can't, then we might be able to get away with prepending all the
>>> encrypted names with some legal characte. Then when we go to decrypt it
>>> we just strip that off.
>> This is probably the best way to fix it, but it's worth trying to find
>> out the origins of this limitation.  I do seem to remember some obscure
>> reasons, related with the long snap names (for which Xiubo has a patch),
>> which will start with '_'.  But yeah I'll have to go dig deeper.
>
> It will recognize the encrypted "_XYZ_${DIGIT}" snapshot name as the long
> snapshot name inherited from its parent snap realm, and will parse the
> "${DIGIT}" as an ino in other places.
>
> Maybe in MDS we should fail the request only when snapshot name is in type of
> "_XYZ_${DIGIT}" instead of only "_XYZ", and in client side should also print one
> error or warn log about this ?

I think this will only make the encrypted snapshot creation less likely to
fail, but it's still possible that the base64 encoded of the encrypted
snapshot name to start with '_' and end with '_${DIGIT}'.  Or have I
misunderstood your suggestion?

> This why added the ceph PR[1] to tell the kclient current snapshot name is a
> long snap name in lssnap. So if we can forbid the snap shot name begin with '_'
> it will simple in kclient code to handle the long snap name, or it will be
> complex in both MDS and kclient.

Yeah, that PR doesn't look too bad, but I'm still looking into your
kernel-side patch.  Which adds a *lot* of complexity to this.  This whole
fscrypt thing is really getting on my nerves -- every single step that
seems simple ends up being a huge pain :-/

Xiubo, have you tested your PR (and corresponding kernel changes) with my
patch?  Do they work correctly?  (I'm about to start looking into doing
that myself, so no worries if you didn't tried that.)

Cheers,
-- 
Luís

>
> [1] https://github.com/ceph/ceph/pull/45208
>
> -- Xiubo
>
>
>>> We could also consider changing the base64 routine to use something else
>>> in lieu of '_' but that's more of a hassle.
>> Cheers,
>