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

Jeff Layton
2021-01-28T13:42:06Z

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>