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