Re: [RFC PATCH v4 13/17] ceph: add support to readdir for encrypted filenames

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2021-01-28 at 11:33 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > Add helper functions for buffer management and for decrypting filenames
> > returned by the MDS. Wire those into the readdir codepaths.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/crypto.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/ceph/crypto.h | 41 ++++++++++++++++++++++++++
> >  fs/ceph/dir.c    | 62 +++++++++++++++++++++++++++++++--------
> >  fs/ceph/inode.c  | 38 ++++++++++++++++++++++--
> >  4 files changed, 202 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > index f037a4939026..7ddd434c5baf 100644
> > --- a/fs/ceph/crypto.c
> > +++ b/fs/ceph/crypto.c
> > @@ -107,3 +107,79 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
> >  		ceph_pagelist_release(pagelist);
> >  	return ret;
> >  }
> > +
> > +/**
> > + * ceph_fname_to_usr - convert a filename for userland presentation
> > + * @fname: ceph_fname to be converted
> > + * @tname: temporary name buffer to use for conversion (may be NULL)
> > + * @oname: where converted name should be placed
> > + * @is_nokey: set to true if key wasn't available during conversion (may be NULL)
> > + *
> > + * Given a filename (usually from the MDS), format it for presentation to
> > + * userland. If @parent is not encrypted, just pass it back as-is.
> > + *
> > + * Otherwise, base64 decode the string, and then ask fscrypt to format it
> > + * for userland presentation.
> > + *
> > + * Returns 0 on success or negative error code on error.
> > + */
> > +int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> > +		      struct fscrypt_str *oname, bool *is_nokey)
> > +{
> > +	int ret;
> > +	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
> > +	struct fscrypt_str iname;
> > +
> > +	if (!IS_ENCRYPTED(fname->dir)) {
> > +		oname->name = fname->name;
> > +		oname->len = fname->name_len;
> > +		return 0;
> > +	}
> > +
> > +	/* Sanity check that the resulting name will fit in the buffer */
> > +	if (fname->name_len > FSCRYPT_BASE64_CHARS(NAME_MAX))
> > +		return -EIO;
> > +
> > +	ret = __fscrypt_prepare_readdir(fname->dir);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Use the raw dentry name as sent by the MDS instead of
> > +	 * generating a nokey name via fscrypt.
> > +	 */
> > +	if (!fscrypt_has_encryption_key(fname->dir)) {
> 
> While chasing a different the bug (the one I mention yesterday on IRC), I
> came across this memory leak: oname->name needs to be freed here.  I
> believe that a
> 
> 	fscrypt_fname_free_buffer(oname);
> 
> before the kmemdup() below should be enough.
> 

Good catch. Thanks.

In hindsight, I'm regretting threading the use of fscrypt_str's through
this code. It's a rather cumbersome object, and I think I might be
better served using a different scheme. Let me think on it some...
-- 
Jeff Layton <jlayton@kernel.org>