netdev - Re: [PATCH bpf-next 2/3] bpf: Keep im address consistent between dry run and real patching

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: <CAPhsuW67c8NYxBhwrq8JK8HP95P1Wwq1zHEDqooAOgP+aru13g@mail.gmail.com>
Date: Mon, 29 Jan 2024 09:58:00 -0800
From: Song Liu <song@...nel.org>
To: Pu Lehui <pulehui@...weicloud.com>
Cc: bpf@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	netdev@...r.kernel.org, Björn Töpel <bjorn@...nel.org>, 
	Puranjay Mohan <puranjay12@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Yonghong Song <yhs@...com>, 
	John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Luke Nelson <luke.r.nels@...il.com>, 
	Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf-next 2/3] bpf: Keep im address consistent between dry
 run and real patching

On Tue, Jan 23, 2024 at 2:32 AM Pu Lehui <pulehui@...weicloud.com> wrote:
>
> From: Pu Lehui <pulehui@...wei.com>
>
> In __arch_prepare_bpf_trampoline, we emit instructions to store the
> address of im to register and then pass it to __bpf_tramp_enter and
> __bpf_tramp_exit functions. Currently we use fake im in
> arch_bpf_trampoline_size for the dry run, and then allocate new im for
> the real patching. This is fine for architectures that use fixed
> instructions to generate addresses. However, for architectures that use
> dynamic instructions to generate addresses, this may make the front and
> rear images inconsistent, leading to patching overflow. We can extract
> the im allocation ahead of the dry run and pass the allocated im to
> arch_bpf_trampoline_size, so that we can ensure that im is consistent in
> dry run and real patching.

IIUC, this is required because emit_imm() for riscv may generate variable
size instructions (depends on the value of im). I wonder we can fix this by
simply set a special value for fake im in arch_bpf_trampoline_size() to
so that emit_imm() always gives biggest value for the fake im.

>
> Signed-off-by: Pu Lehui <pulehui@...wei.com>
> ---
[...]
>
>  static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex)
> @@ -432,23 +425,27 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>                 tr->flags |= BPF_TRAMP_F_ORIG_STACK;
>  #endif
>
> -       size = arch_bpf_trampoline_size(&tr->func.model, tr->flags,
> +       im = kzalloc(sizeof(*im), GFP_KERNEL);
> +       if (!im) {
> +               err = -ENOMEM;
> +               goto out;
> +       }
> +
> +       size = arch_bpf_trampoline_size(im, &tr->func.model, tr->flags,
>                                         tlinks, tr->func.addr);
>         if (size < 0) {
>                 err = size;
> -               goto out;
> +               goto out_free_im;
>         }
>
>         if (size > PAGE_SIZE) {
>                 err = -E2BIG;
> -               goto out;
> +               goto out_free_im;
>         }
>
> -       im = bpf_tramp_image_alloc(tr->key, size);
> -       if (IS_ERR(im)) {
> -               err = PTR_ERR(im);
> -               goto out;
> -       }
> +       err = bpf_tramp_image_alloc(im, tr->key, size);
> +       if (err < 0)
> +               goto out_free_im;

I feel this change just makes bpf_trampoline_update() even
more confusing.

>
>         err = arch_prepare_bpf_trampoline(im, im->image, im->image + size,
>                                           &tr->func.model, tr->flags, tlinks,
> @@ -496,6 +493,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>
>  out_free:
>         bpf_tramp_image_free(im);
> +out_free_im:
> +       kfree_rcu(im, rcu);

If we goto out_free above, we will call kfree_rcu(im, rcu)
twice, right? Once in bpf_tramp_image_free(), and again
here.

Thanks,
Song

[...]