Re: [PATCH 1/2] ceph: pass fscrypt `tname` buffers directly

[Sam Edwards]

On Wed, May 27, 2026 at 3:13 PM David Laight
<david.laight.linux@xxxxxxxxx> wrote:
>
> On Wed, 27 May 2026 11:06:08 -0700
> Sam Edwards <cfsworks@xxxxxxxxx> wrote:
>
> > On Wed, May 27, 2026 at 5:00 AM David Laight
> > <david.laight.linux@xxxxxxxxx> wrote:
> > >
> > > On Tue, 26 May 2026 19:58:27 -0700
> > > Sam Edwards <cfsworks@xxxxxxxxx> wrote:
> > >
> > > > ceph_fname_to_usr() needs a temporary buffer for some operations
> > > > (currently only base64-decoding ciphertext) and it is convenient to
> > > > allow the caller to specify this buffer to avoid a heap allocation, so
> > > > it has a (nullable) `tname` argument. Until now, this argument was a
> > > > `struct fscrypt_str`; however, this is unnecessary for two reasons:
> > > >
> > > > 1. `tname->len` isn't used anywhere: ceph_fname_to_usr() assumes a
> > > >    buffer large enough to hold the ciphertext, and
> > > >    parse_reply_info_readdir() -- the only caller to use tname -- doesn't
> > > >    set it.
> > > > 2. While the `tname` parameter is documented "may be NULL,"
> > > >    parse_reply_info_readdir() always passes it but with `tname->name`
> > > >    sometimes NULL in violation of the contract, indicating that the
> > > >    unnecessary container creates actual confusion.
> > > >
> > > > Therefore, change the type to `unsigned char *` and pass the buffer
> > > > directly.
> > > >
> > > > Signed-off-by: Sam Edwards <CFSworks@xxxxxxxxx>
> > > > ---
> > > >  fs/ceph/crypto.c     | 10 +++++-----
> > > >  fs/ceph/crypto.h     |  4 ++--
> > > >  fs/ceph/mds_client.c |  6 +++---
> > > >  3 files changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > index 64d240759277..7515cb251226 100644
> > > > --- a/fs/ceph/crypto.c
> > > > +++ b/fs/ceph/crypto.c
> > > > @@ -300,7 +300,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > >   *
> > > >   * Returns 0 on success or negative error code on error.
> > > >   */
> > > > -int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> > > > +int ceph_fname_to_usr(const struct ceph_fname *fname, unsigned char *tname,
> > >
> > > I can't help feeling that the buffer length should also be passed.
> > > Either explicitly or, if constant, implicitly by embedding the array
> > > in a structure.
> >
> > It isn't constant; the specific requirement (unchanged in patch 2) is
> > that the buffer be at least large enough to hold the ciphertext. The
> > only caller to pass tname has a comment explaining how it meets the
> > size requirement, so this is currently safe.
>
> Ugg...
> That is just an accident waiting to happen.
>
> > Or is your feeling more about general robustness, ensuring that the
> > function prototype of ceph_fname_to_usr() makes it hard for future
> > patches to ignore the length requirement? If so, the issue is
> > ultimately that the base64_*() functions don't accept a `dstlen` that
> > ceph_fname_to_usr() could use to (meaningfully) enforce the size
> > requirement.
>
> The output for the base64 functions depends only on the size of the
> input - so is easy to get right.
> And for decode is always shorter.

That's the same brand of "easy to get right" as passing a big-enough
tname buffer. In fact, you just paraphrased the second sentence of the
safety comment in parse_reply_info_readdir().

But my concern is that if tname->len were to be made significant --
which I'm receptive to doing, to be clear -- I need to be able to
enforce that length meaningfully and proactively, which
base64_decode() currently doesn't allow. And something like this is a
non-starter:

declen = base64_decode(name, name_len,
                       tname->name, false, BASE64_IMAP);
if (declen <= 0 || declen > tname->len) {
    ret = -EIO;
    goto out;
}

And I'm firmly against mixing the "explicit buffer bounds check" and
"easy enough to plan the size ahead of time" approaches; that's the
real "accident waiting to happen" territory. Any argument for an
explicit check here is also an argument for an explicit check in
base64_{de,en}code().

> I've just looked at what happens when tname is NULL - that looks
> broken as well - why not just kmalloc() a buffer that is the right
> size instead of using a wrapper function that might return a
> different length entirely.
> Maybe it should be too long, but bugs happen.

The wrapper function guarantees a minimum length, and it's being
called with NAME_MAX, so it's (at least) big enough to hold the
longest possible value it needs to hold. What kind of bug are you
expecting, should the buffer be too long?

> There are seem to be random overwrites of buffers of pointers
> to buffers - more code that is badly fragile.

I need more specificity: Where? Which buffers are being overwritten?
Under which circumstances? Which code?

Best,
Sam

>
> -- David
>
> >
> > Cheers,
> > Sam
> >
> > >
> > > -- David
> > >
> > >
> > > >                     struct fscrypt_str *oname, bool *is_nokey)
> > > >  {
> > > >       struct inode *dir = fname->dir;
> > > > @@ -357,16 +357,16 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> > > >                       ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
> > > >                       if (ret)
> > > >                               goto out_inode;
> > > > -                     tname = &_tname;
> > > > +                     tname = _tname.name;
> > > >               }
> > > >
> > > > -             declen = base64_decode(name, name_len,
> > > > -                                    tname->name, false, BASE64_IMAP);
> > > > +             declen = base64_decode(name, name_len, tname, false,
> > > > +                                    BASE64_IMAP);
> > > >               if (declen <= 0) {
> > > >                       ret = -EIO;
> > > >                       goto out;
> > > >               }
> > > > -             iname.name = tname->name;
> > > > +             iname.name = tname;
> > > >               iname.len = declen;
> > > >       } else {
> > > >               iname.name = fname->ctext;
> > > > diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> > > > index b748e2060bc9..79cb563fd887 100644
> > > > --- a/fs/ceph/crypto.h
> > > > +++ b/fs/ceph/crypto.h
> > > > @@ -115,7 +115,7 @@ static inline void ceph_fname_free_buffer(struct inode *parent,
> > > >               fscrypt_fname_free_buffer(fname);
> > > >  }
> > > >
> > > > -int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> > > > +int ceph_fname_to_usr(const struct ceph_fname *fname, unsigned char *tname,
> > > >                     struct fscrypt_str *oname, bool *is_nokey);
> > > >  int ceph_fscrypt_prepare_readdir(struct inode *dir);
> > > >
> > > > @@ -204,7 +204,7 @@ static inline void ceph_fname_free_buffer(struct inode *parent,
> > > >  }
> > > >
> > > >  static inline int ceph_fname_to_usr(const struct ceph_fname *fname,
> > > > -                                 struct fscrypt_str *tname,
> > > > +                                 unsigned char *tname,
> > > >                                   struct fscrypt_str *oname, bool *is_nokey)
> > > >  {
> > > >       oname->name = fname->name;
> > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > index ed17e0023705..aa6730b48e97 100644
> > > > --- a/fs/ceph/mds_client.c
> > > > +++ b/fs/ceph/mds_client.c
> > > > @@ -488,11 +488,11 @@ static int parse_reply_info_readdir(void **p, void *end,
> > > >               struct inode *inode = d_inode(req->r_dentry);
> > > >               struct ceph_inode_info *ci = ceph_inode(inode);
> > > >               struct ceph_mds_reply_dir_entry *rde = info->dir_entries + i;
> > > > -             struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > >               struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > >               struct ceph_fname fname;
> > > >               u32 altname_len, _name_len;
> > > >               u8 *altname, *_name;
> > > > +             u8 *tname = NULL;
> > > >
> > > >               /* dentry */
> > > >               ceph_decode_32_safe(p, end, _name_len, bad);
> > > > @@ -540,7 +540,7 @@ static int parse_reply_info_readdir(void **p, void *end,
> > > >                        * always be shorter, which is 3/4 of origin
> > > >                        * string.
> > > >                        */
> > > > -                     tname.name = _name;
> > > > +                     tname = _name;
> > > >
> > > >                       /*
> > > >                        * Set oname to _name too, and this will be
> > > > @@ -557,7 +557,7 @@ static int parse_reply_info_readdir(void **p, void *end,
> > > >                       oname.len = altname_len;
> > > >               }
> > > >               rde->is_nokey = false;
> > > > -             err = ceph_fname_to_usr(&fname, &tname, &oname, &rde->is_nokey);
> > > > +             err = ceph_fname_to_usr(&fname, tname, &oname, &rde->is_nokey);
> > > >               if (err) {
> > > >                       pr_err_client(cl, "unable to decode %.*s, got %d\n",
> > > >                                     _name_len, _name, err);
> > >
>