linux-kernel - Re: [PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOi1vP_G9ybNs_QEn34cPvovAa=JB7G9F3FGy33QPH4yfST-iQ@mail.gmail.com>
Date:   Tue, 25 Jun 2019 16:35:59 +0200
From:   Ilya Dryomov <idryomov@...il.com>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ceph Development <ceph-devel@...r.kernel.org>,
        "Yan, Zheng" <zyan@...hat.com>, Sage Weil <sage@...hat.com>,
        agruenba@...hat.com
Subject: Re: [PATCH v4 3/3] ceph: don't NULL terminate virtual xattrs

On Mon, Jun 24, 2019 at 6:27 PM Jeff Layton <jlayton@...nel.org> wrote:
>
> The convention with xattrs is to not store the termination with string
> data, given that it returns the length. This is how setfattr/getfattr
> operate.
>
> Most of ceph's virtual xattr routines use snprintf to plop the string
> directly into the destination buffer, but snprintf always NULL
> terminates the string. This means that if we send the kernel a buffer
> that is the exact length needed to hold the string, it'll end up
> truncated.
>
> Add a ceph_fmt_xattr helper function to format the string into an
> on-stack buffer that is should always be large enough to hold the whole
> thing and then memcpy the result into the destination buffer. If it does
> turn out that the formatted string won't fit in the on-stack buffer,
> then return -E2BIG and do a WARN_ONCE().
>
> Change over most of the virtual xattr routines to use the new helper. A
> couple of the xattrs are sourced from strings however, and it's
> difficult to know how long they'll be. Just have those memcpy the result
> in place after verifying the length.
>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  fs/ceph/xattr.c | 84 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 9b77dca0b786..37b458a9af3a 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -109,22 +109,49 @@ static ssize_t ceph_vxattrcb_layout(struct ceph_inode_info *ci, char *val,
>         return ret;
>  }
>
> +/*
> + * The convention with strings in xattrs is that they should not be NULL
> + * terminated, since we're returning the length with them. snprintf always
> + * NULL terminates however, so call it on a temporary buffer and then memcpy
> + * the result into place.
> + */
> +static int ceph_fmt_xattr(char *val, size_t size, const char *fmt, ...)
> +{
> +       int ret;
> +       va_list args;
> +       char buf[96]; /* NB: reevaluate size if new vxattrs are added */
> +
> +       va_start(args, fmt);
> +       ret = vsnprintf(buf, size ? sizeof(buf) : 0, fmt, args);
> +       va_end(args);
> +
> +       /* Sanity check */
> +       if (size && ret + 1 > sizeof(buf)) {
> +               WARN_ONCE(true, "Returned length too big (%d)", ret);
> +               return -E2BIG;
> +       }
> +
> +       if (ret <= size)
> +               memcpy(val, buf, ret);
> +       return ret;
> +}

Nit: perhaps check size at the top and bail early instead of checking
it at every step?

Thanks,

                Ilya