netdev - Re: [PATCH 10/19] bpf: Allow to store caller's ip as argument

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAEf4BzZH5ck1Do7oRxP9D5U2v659tFXNW2RfCYAQXV_d2dYc4g@mail.gmail.com>
Date:   Tue, 8 Jun 2021 11:49:31 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        "Steven Rostedt (VMware)" <rostedt@...dmis.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, Daniel Xu <dxu@...uu.xyz>,
        Viktor Malik <vmalik@...hat.com>
Subject: Re: [PATCH 10/19] bpf: Allow to store caller's ip as argument

On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> When we will have multiple functions attached to trampoline
> we need to propagate the function's address to the bpf program.
>
> Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
> function that will store origin caller's address before function's
> arguments.
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
>  include/linux/bpf.h         |  5 +++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b77e6bd78354..d2425c18272a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>                                 void *orig_call)
>  {
>         int ret, i, cnt = 0, nr_args = m->nr_args;
> -       int stack_size = nr_args * 8;
> +       int stack_size = nr_args * 8, ip_arg = 0;
>         struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
>         struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
>         struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
> @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>                  */
>                 orig_call += X86_PATCH_SIZE;
>
> +       if (flags & BPF_TRAMP_F_IP_ARG)
> +               stack_size += 8;
> +

nit: move it a bit up where we adjust stack_size for BPF_TRAMP_F_CALL_ORIG flag?

>         prog = image;
>
>         EMIT1(0x55);             /* push rbp */
> @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>         EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */
>         EMIT1(0x53);             /* push rbx */
>
> -       save_regs(m, &prog, nr_args, stack_size);
> +       if (flags & BPF_TRAMP_F_IP_ARG) {
> +               emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> +               EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/
> +               emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size);
> +               ip_arg = 8;
> +       }

why not pass flags into save_regs and let it handle this case without
this extra ip_arg adjustment?

> +
> +       save_regs(m, &prog, nr_args, stack_size - ip_arg);
>
>         if (flags & BPF_TRAMP_F_CALL_ORIG) {
>                 /* arg1: mov rdi, im */
> @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>         }
>
>         if (flags & BPF_TRAMP_F_CALL_ORIG) {
> -               restore_regs(m, &prog, nr_args, stack_size);
> +               restore_regs(m, &prog, nr_args, stack_size - ip_arg);
>

similarly (and symmetrically), pass flags into restore_regs() to
handle that ip_arg transparently?

>                 if (flags & BPF_TRAMP_F_ORIG_STACK) {
>                         emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>                 }
>
>         if (flags & BPF_TRAMP_F_RESTORE_REGS)
> -               restore_regs(m, &prog, nr_args, stack_size);
> +               restore_regs(m, &prog, nr_args, stack_size - ip_arg);
>
>         /* This needs to be done regardless. If there were fmod_ret programs,
>          * the return value is only updated on the stack and still needs to be
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 16fc600503fb..6cbf3c81c650 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -559,6 +559,11 @@ struct btf_func_model {
>   */
>  #define BPF_TRAMP_F_ORIG_STACK         BIT(3)
>
> +/* First argument is IP address of the caller. Makes sense for fentry/fexit
> + * programs only.
> + */
> +#define BPF_TRAMP_F_IP_ARG             BIT(4)
> +
>  /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
>   * bytes on x86.  Pick a number to fit into BPF_IMAGE_SIZE / 2
>   */
> --
> 2.31.1
>