Message-ID: <15676ff5-5c5c-fd06-308f-10611c01f6a9@eho.link>
Date:   Thu, 9 Dec 2021 18:55:20 +0100
From:   Emmanuel Deloget <emmanuel.deloget@....link>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Björn Töpel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 bpf 1/1] libbpf: don't force user-supplied ifname
 string to be of fixed size

Hello,

On 09/12/2021 18:17, Andrii Nakryiko wrote:
> On Thu, Dec 9, 2021 at 4:03 AM Emmanuel Deloget
> <emmanuel.deloget@....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.

I cannot be extremely precise on what was happening as I did not 
investigate past this (and this fixes our issue) but I suspect that 
having weird bytes in ctx->ifname polutes ifr.ifr_name as initialized in 
xsk_get_max_queues(). ioctl(SIOCETHTOOL) was then giving us an error. 
Now, I haven't looked how the kernel implements this ioctl() so I'm not 
going to say that there is a problem here as well.

And since the issue is now about 2 weeks old it's now a bit murky - and 
I don't have much time to put myself in the same setup in order to 
produce a better investigation (sorry for that).

>>
>> Fixes: 2f6324a3937f8 (libbpf: Support shared umems between queues and devices)
>> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@....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?)

Well, strncpy() calls were replaced by memcpy() a while ago (see 
3015b500ae42 (libbpf: Use memcpy instead of strncpy to please GCC) for 
example but there are a few other examples ; most of the changes were 
made to please gcc8) so I thought that it would be a bad idea :). What 
would be the consensus on this?

Regarding '\0', I'll change that.

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

Will do.

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

BTW, is there a reason why this patch failed to pass the bpf/vmtest-bpf 
test on patchwork?

Best regards,

-- Emmanuel Deloget

Powered by blists - more mailing lists