netdev - Re: [PATCH bpf-next] libbpf: fix crash in XDP socket part with new larger BPF_LOG_BUF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5efdf57f-7746-edfc-25a2-6f768a493747@iogearbox.net>
Date:   Tue, 9 Apr 2019 23:44:08 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Magnus Karlsson <magnus.karlsson@...el.com>, bjorn.topel@...el.com,
        ast@...nel.org, netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next] libbpf: fix crash in XDP socket part with new
 larger BPF_LOG_BUF_SIZE

On 04/09/2019 02:49 PM, Magnus Karlsson wrote:
> In commit da11b417583e ("libbpf: teach libbpf about log_level bit 2"),
> the BPF_LOG_BUF_SIZE was increased to 16M. The XDP socket part of
> libbpf allocated the log_buf on the stack, but for the new 16M buffer
> size this is not going to work. Change the code to allocate the
> log_buf using malloc instead. If the allocation fails, we go ahead and
> load the program without a log buffer.
> 
> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>

For these few 17 insn, wouldn't it make more sense to rather shrink the
log buffer size? malloc'ing 16M seems like overkill for the tiny program.
I don't think there is a specific need to stick with BPF_LOG_BUF_SIZE.
Why not simplifying it like the following where we also don't need to
cope will allocation failure, for example:

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 8d0078b..557ef8d 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -259,7 +259,8 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,

 static int xsk_load_xdp_prog(struct xsk_socket *xsk)
 {
-       char bpf_log_buf[BPF_LOG_BUF_SIZE];
+       static const int log_buf_size = 16 * 1024;
+       char log_buf[log_buf_size];
        int err, prog_fd;

        /* This is the C-program:
@@ -308,10 +309,10 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
        size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);

        prog_fd = bpf_load_program(BPF_PROG_TYPE_XDP, prog, insns_cnt,
-                                  "LGPL-2.1 or BSD-2-Clause", 0, bpf_log_buf,
-                                  BPF_LOG_BUF_SIZE);
+                                  "LGPL-2.1 or BSD-2-Clause", 0, log_buf,
+                                  log_buf_size);
        if (prog_fd < 0) {
-               pr_warning("BPF log buffer:\n%s", bpf_log_buf);
+               pr_warning("BPF log buffer:\n%s", log_buf);
                return prog_fd;
        }

Thanks,
Daniel