lkml.org 
[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v1 bpf 1/1] libbpf: don't force user-supplied ifname string to be of fixed size
From
Hello,

On 09/12/2021 18:17, Andrii Nakryiko wrote:
> 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.

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@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?)

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

\
 
 \ /
  Last update: 2021-12-09 18:57    [W:0.040 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site