netdev - Re: [PATCH bpf-next 1/3] bpf, x64: Replace some stack

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]

Date:   Mon, 6 Dec 2021 13:41:15 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Andrii Nakryiko <andrii@...nel.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>
Subject: Re: [PATCH bpf-next 1/3] bpf, x64: Replace some stack_size usage with
 offset variables

On Sat, Dec 4, 2021 at 6:07 AM Jiri Olsa <jolsa@...hat.com> wrote:
>
> As suggested by Andrii, adding variables for registers and ip
> address offsets, which makes the code more clear, rather than
> abusing single stack_size variable for everything.
>
> Also describing the stack layout in the comment.
>
> There is no function change.
>
> Suggested-by: Andrii Nakryiko <andrii@...nel.org>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 42 ++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 1d7b0c69b644..b106e80e8d9c 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1941,7 +1941,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>                                 void *orig_call)
>  {
>         int ret, i, nr_args = m->nr_args;
> -       int stack_size = nr_args * 8;
> +       int regs_off, ip_off, stack_size = nr_args * 8;
>         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];
> @@ -1956,14 +1956,33 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>         if (!is_valid_bpf_tramp_flags(flags))
>                 return -EINVAL;
>
> +       /* Generated trampoline stack layout:
> +        *
> +        * RBP + 8         [ return address  ]
> +        * RBP + 0         [ RBP             ]
> +        *
> +        * RBP - 8         [ return value    ]  BPF_TRAMP_F_CALL_ORIG or
> +        *                                      BPF_TRAMP_F_RET_FENTRY_RET flags
> +        *
> +        *                 [ reg_argN        ]  always
> +        *                 [ ...             ]
> +        * RBP - regs_off  [ reg_arg1        ]
> +        *

I think it's also worth mentioning that context passed into
fentry/fexit programs are pointing here (makes it a bit easier to
track those ctx[-1] and ctx[-2] in the next patch.


> +        * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
> +        */
> +
>         /* room for return value of orig_call or fentry prog */
>         save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
>         if (save_ret)
>                 stack_size += 8;
>
> +       regs_off = stack_size;
> +
>         if (flags & BPF_TRAMP_F_IP_ARG)
>                 stack_size += 8; /* room for IP address argument */
>
> +       ip_off = stack_size;
> +

[...]