Messages in this thread | | | Date | Thu, 9 Dec 2021 18:55:20 +0100 | Subject | Re: [PATCH v1 bpf 1/1] libbpf: don't force user-supplied ifname string to be of fixed size | From | Emmanuel Deloget <> |
| |
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
| |