On Thu, Dec 9, 2021 at 9:55 AM Emmanuel Deloget
<emmanuel.deloget@xxxxxxxx> wrote:
>
> 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).
>
Ok, fine, but I still don't see how memcpy() could have caused
correctness errors. The string will be zero-terminated properly, so it
is a valid C string. The only issue I see is reading past allocated
memory (which might trigger SIGSEGV or will make ASAN complain).
Anyways, let's try strncpy() and fix this. Please send it against
bpf-next for the respin, please.
> >>
> >> 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?
3015b500ae42 ("libbpf: Use memcpy instead of strncpy to please GCC")
is different, there we are copying from properly sized array which our
code controls. memcpy() is totally reasonable there. Here we can't do
memcpy, unfortunately. Let's try strncpy(), if GCC will start
complaining about this, then GCC is clearly wrong and we'll just
disable this warning altogether (I don't remember if it ever found any
real issues anyways).
>
> 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?
>
Unrelated bpftool-related check, that isn't supposed to pass on bpf
tree. That one can be ignored.
> Best regards,
>
> -- Emmanuel Deloget
