MIME-Version: 1.0
References: <20211209120327.551952-1-emmanuel.deloget@eho.link>
In-Reply-To: <20211209120327.551952-1-emmanuel.deloget@eho.link>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Thu, 9 Dec 2021 09:17:24 -0800
Message-ID: <CAEf4BzYJ+GPpjcMMYQM_BfQ1-aq6dz_JbF-m5meiCZ=oPbrM=w@mail.gmail.com>
Subject: Re: [PATCH v1 bpf 1/1] libbpf: don't force user-supplied ifname
 string to be of fixed size
To:     Emmanuel Deloget <emmanuel.deloget@eho.link>
Cc:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 9, 2021 at 4:03 AM Emmanuel Deloget
<emmanuel.deloget@eho.link> wrote:
>
> When calling either xsk_socket__create_shared() or xsk_socket__create()
> the user supplies a const char *ifname which is implicitely supposed to
> be a pointer to the start of a char[IFNAMSIZ] array. The internal
> function xsk_create_ctx() then blindly copy IFNAMSIZ bytes from this
> string into the xsk context.
>
> This is counter-intuitive and error-prone.
>
> For example,
>
>         int r = xsk_socket__create(..., "eth0", ...)
>
> may result in an invalid object because of the blind copy. The "eth0"
> string might be followed by random data from the ro data section,
> resulting in ctx->ifname being filled with the correct interface name
> then a bunch and invalid bytes.
>
> The same kind of issue arises when the ifname string is located on the
> stack:
>
>         char ifname[] = "eth0";
>         int r = xsk_socket__create(..., ifname, ...);
>
> Or comes from the command line
>
>         const char *ifname = argv[n];
>         int r = xsk_socket__create(..., ifname, ...);
>
> In both case we'll fill ctx->ifname with random data from the stack.
>
> In practice, we saw that this issue caused various small errors which,
> in then end, prevented us to setup a valid xsk context that would have
> allowed us to capture packets on our interfaces. We fixed this issue in
> our code by forcing our char ifname[] to be of size IFNAMSIZ but that felt
> weird and unnecessary.

I might be missing something, but the eth0 example above would include
terminating zero at the right place, so ifname will still have
"eth0\0" which is a valid string. Yes there will be some garbage after
that, but it shouldn't matter. It could cause ASAN to complain about
reading beyond allocated memory, of course, but I'm curious what
problems you actually ran into in practice.

>
> Fixes: 2f6324a3937f8 (libbpf: Support shared umems between queues and devices)
> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
> ---
>  tools/lib/bpf/xsk.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 81f8fbc85e70..8dda80bcefcc 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -944,6 +944,7 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
>  {
>         struct xsk_ctx *ctx;
>         int err;
> +       size_t ifnamlen;
>
>         ctx = calloc(1, sizeof(*ctx));
>         if (!ctx)
> @@ -965,8 +966,10 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
>         ctx->refcount = 1;
>         ctx->umem = umem;
>         ctx->queue_id = queue_id;
> -       memcpy(ctx->ifname, ifname, IFNAMSIZ - 1);
> -       ctx->ifname[IFNAMSIZ - 1] = '\0';
> +
> +       ifnamlen = strnlen(ifname, IFNAMSIZ);
> +       memcpy(ctx->ifname, ifname, ifnamlen);

maybe use strncpy instead of strnlen + memcpy? keep the guaranteed
zero termination (and keep '\0', why did you change it?)

Also, note that xsk.c is deprecated in libbpf and has been moved into
libxdp, so please contribute a similar fix there.

> +       ctx->ifname[IFNAMSIZ - 1] = 0;
>
>         ctx->fill = fill;
>         ctx->comp = comp;
> --
> 2.32.0
>