On Tue, 2025-02-18 at 23:52 +0000, Al Viro wrote:
> On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:
>
> > See the problem? strrchr() expects a NUL-terminated string; giving it an
> > array that has no zero bytes in it is an UB.
> >
> > That one is -stable fodder on its own, IMO...
>
> FWIW, it's more unpleasant; there are other call chains for parse_longname()
> where it's not feasible to NUL-terminate in place. I suspect that the
> patch below is a better way to handle that. Comments?
>
Let me test the patch.
> From ed016e5ea89550b567306207ba3ca8b60e147d89 Mon Sep 17 00:00:00 2001
> From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Date: Tue, 18 Feb 2025 17:57:17 -0500
> Subject: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated
> string
>
> ... and parse_longname() is not guaranteed that. That's the reason
> why it uses kmemdup_nul() to build the argument for kstrtou64();
> the problem is, kstrtou64() is not the only thing that need it.
>
> Just get a NUL-terminated copy of the entire thing and be done
> with that...
>
> Fixes: dd66df0053ef "ceph: add support for encrypted snapshot names"
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> fs/ceph/crypto.c | 32 ++++++++++++--------------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 3b3c4d8d401e..164e7981aecb 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -215,35 +215,30 @@ static struct inode *parse_longname(const struct inode *parent,
> struct ceph_client *cl = ceph_inode_to_client(parent);
> struct inode *dir = NULL;
> struct ceph_vino vino = { .snap = CEPH_NOSNAP };
> - char *inode_number;
> char *name_end;
> - int orig_len = *name_len;
> int ret = -EIO;
> -
> + /* NUL-terminate */
> + char *s __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
Maybe, str here? The s looks too extreme for my taste. :)
> + if (!s)
> + return ERR_PTR(-ENOMEM);
> /* Skip initial '_' */
> - name++;
> - name_end = strrchr(name, '_');
> + s++;
> + name_end = strrchr(s, '_');
> if (!name_end) {
> - doutc(cl, "failed to parse long snapshot name: %s\n", name);
> + doutc(cl, "failed to parse long snapshot name: %s\n", s);
> return ERR_PTR(-EIO);
> }
> - *name_len = (name_end - name);
> + *name_len = (name_end - s);
> if (*name_len <= 0) {
> pr_err_client(cl, "failed to parse long snapshot name\n");
> return ERR_PTR(-EIO);
> }
>
> /* Get the inode number */
> - inode_number = kmemdup_nul(name_end + 1,
> - orig_len - *name_len - 2,
> - GFP_KERNEL);
> - if (!inode_number)
> - return ERR_PTR(-ENOMEM);
> - ret = kstrtou64(inode_number, 10, &vino.ino);
> + ret = kstrtou64(name_end + 1, 10, &vino.ino);
The inode_number explains by itself what we are converting here. But name_end
sounds confusing and not informative for my taste. But I could have a bad taste.
:)
> if (ret) {
> - doutc(cl, "failed to parse inode number: %s\n", name);
> - dir = ERR_PTR(ret);
> - goto out;
> + doutc(cl, "failed to parse inode number: %s\n", s);
> + return ERR_PTR(ret);
> }
>
> /* And finally the inode */
> @@ -252,11 +247,8 @@ static struct inode *parse_longname(const struct inode *parent,
> /* This can happen if we're not mounting cephfs on the root */
> dir = ceph_get_inode(parent->i_sb, vino, NULL);
> if (IS_ERR(dir))
> - doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
> + doutc(cl, "can't find inode %s (%s)\n", name_end + 1, name);
It's not critical, but we use name_end + 1 two times. Potentially, it could be a
source of somebody mistake in the future.
Thanks,
Slava.
> }
> -
> -out:
> - kfree(inode_number);
> return dir;
> }
>
