Date: Mon, 5 Jun 2023 16:30:29 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Krister Johansen <kjlx@...pleofstupid.com>
Cc: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, 
	Yonghong Song <yhs@...com>, KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, 
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Tom Rix <trix@...hat.com>, 
	LKML <linux-kernel@...r.kernel.org>, Network Development <netdev@...r.kernel.org>, 
	clang-built-linux <llvm@...ts.linux.dev>, stable <stable@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: search_bpf_extables should search subprogram extables

On Mon, Jun 5, 2023 at 9:50 AM Krister Johansen <kjlx@...pleofstupid.com> wrote:
>
> JIT'd bpf programs that have subprograms can have a postive value for
> num_extentries but a NULL value for extable.  This is problematic if one of
> these bpf programs encounters a fault during its execution.  The fault
> handlers correctly identify that the faulting IP belongs to a bpf program.
> However, performing a search_extable call on a NULL extable leads to a
> second fault.
>
> Fix up by refusing to search a NULL extable, and by checking the
> subprograms' extables if the umbrella program has subprograms configured.
>
> Once I realized what was going on, I was able to use the following bpf
> program to get an oops from this failure:
>
>    #include "vmlinux.h"
>    #include <bpf/bpf_helpers.h>
>    #include <bpf/bpf_tracing.h>
>
>    char LICENSE[] SEC("license") = "Dual BSD/GPL";
>
>    #define PATH_MAX 4096
>
>    struct callback_ctx {
>            u8 match;
>    };
>
>    struct filter_value {
>            char prefix[PATH_MAX];
>    };
>    struct {
>            __uint(type, BPF_MAP_TYPE_ARRAY);
>            __uint(max_entries, 256);
>            __type(key, int);
>            __type(value, struct filter_value);
>    } test_filter SEC(".maps");
>
>    static __u64 test_filter_cb(struct bpf_map *map, __u32 *key,
>                                struct filter_value *val,
>                                struct callback_ctx *data)
>    {
>        return 1;
>    }
>
>    SEC("fentry/__sys_bind")
>    int BPF_PROG(__sys_bind, int fd, struct sockaddr *umyaddr, int addrlen)
>    {
>      pid_t pid;
>
>      struct callback_ctx cx = { .match = 0 };
>      pid = bpf_get_current_pid_tgid() >> 32;
>      bpf_for_each_map_elem(&test_filter, test_filter_cb, &cx, 0);
>      bpf_printk("fentry: pid = %d, family = %llx\n", pid, umyaddr->sa_family);

Instead of printk please do a volatile read of umyaddr->sa_family.

Please convert this commit log to a test in selftest/bpf/
and resubmit as two patches.

Also see bpf_testmod_return_ptr() and
SEC("fexit/bpf_testmod_return_ptr") in progs/test_module_attach.c.

Probably easier to tweak that test for subprogs instead
of adding your own SEC("fentry/__sys_bind") test and triggering bind()
from user space.


>      return 0;
>    }
>
> And then the following code to actually trigger a failure:
>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <unistd.h>
>   #include <sys/socket.h>
>   #include <netinet/in.h>
>   #include <netinet/ip.h>
>
>   int
>   main(int argc, char *argv[])
>   {
>     int sfd, rc;
>     struct sockaddr *sockptr = (struct sockaddr *)0x900000000000;
>
>     sfd = socket(AF_INET, SOCK_STREAM, 0);
>     if (sfd < 0) {
>       perror("socket");
>       exit(EXIT_FAILURE);
>     }
>
>     while (1) {
>       rc = bind(sfd, (struct sockaddr *) sockptr, sizeof(struct sockaddr_in));
>       if (rc < 0) {
>         perror("bind");
>         sleep(5);
>       } else {
>         break;
>       }
>     }
>
>     return 0;
>   }
>
> I was able to validate that this problem does not occur when subprograms
> are not in use, or when the direct pointer accesses are replaced with
> bpf_probe_read calls.  I further validated that this did not break the
> extable handling in existing bpf programs.  The same program caused no
> failures when subprograms were removed, but the exception was still
> injected.
>
> Cc: stable@...r.kernel.org
> Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> Signed-off-by: Krister Johansen <kjlx@...pleofstupid.com>
> ---
>  kernel/bpf/core.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 7421487422d4..0e12238e4340 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -736,15 +736,33 @@ const struct exception_table_entry *search_bpf_extables(unsigned long addr)
>  {
>         const struct exception_table_entry *e = NULL;
>         struct bpf_prog *prog;
> +       struct bpf_prog_aux *aux;
> +       int i;
>
>         rcu_read_lock();
>         prog = bpf_prog_ksym_find(addr);
>         if (!prog)
>                 goto out;
> -       if (!prog->aux->num_exentries)
> +       aux = prog->aux;
> +       if (!aux->num_exentries)
>                 goto out;
>
> -       e = search_extable(prog->aux->extable, prog->aux->num_exentries, addr);
> +       /* prog->aux->extable can be NULL if subprograms are in use. In that
> +        * case, check each sub-function's aux->extables to see if it has a
> +        * matching entry.
> +        */
> +       if (aux->extable != NULL) {
> +               e = search_extable(prog->aux->extable,
> +                   prog->aux->num_exentries, addr);
> +       } else {
> +               for (i = 0; (i < aux->func_cnt) && (e == NULL); i++) {

() are redundant.
!e is preferred over e == NULL

> +                       if (!aux->func[i]->aux->num_exentries ||
> +                           aux->func[i]->aux->extable == NULL)
> +                               continue;
> +                       e = search_extable(aux->func[i]->aux->extable,
> +                           aux->func[i]->aux->num_exentries, addr);
> +               }
> +       }

something odd here.
We do bpf_prog_kallsyms_add(func[i]); for each subprog.
So bpf_prog_ksym_find() in search_bpf_extables()
should be finding ksym and extable of the subprog
and not the main prog.
The bug is probably elsewhere.

Once you respin with a selftest we can help debugging.

Powered by blists - more mailing lists