linux-kernel - Re: [PATCH bpf] bpf: verifier: fix addr_space

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>] [day] [month] [year] [list]

Message-ID: <CANk7y0jJN7D8Df0b9-6TSvpwqMuQFyf8mmtyRBQ6hNtKFU3mvQ@mail.gmail.com>
Date: Thu, 21 Mar 2024 16:38:26 +0100
From: Puranjay Mohan <puranjay12@...il.com>
To: 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>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf] bpf: verifier: fix addr_space_cast from as(1) to as(0)

On Thu, Mar 21, 2024 at 4:03 PM Puranjay Mohan <puranjay12@...il.com> wrote:
>
> The verifier currently converts addr_space_cast from as(1) to as(0) that
> is: BPF_ALU64 | BPF_MOV | BPF_X with off=1 and imm=1
> to
> BPF_ALU | BPF_MOV | BPF_X with imm=1 (32-bit mov)
>
> Because of this imm=1, the JITs that have bpf_jit_needs_zext() == true,
> interpret the converted instruction as BPF_ZEXT_REG(DST) which is a
> special form of mov32, used for doing explicit zero extension on dst.
> These JITs will just zero extend the dst reg and will not move the src to
> dst before the zext.
>
> Fix do_misc_fixups() to set imm=0 when converting addr_space_cast to a
> normal mov32.
>
> The JITs that have bpf_jit_needs_zext() == true rely on the verifier to
> emit zext instructions. Mark dst_reg as subreg when doing cast from
> as(1) to as(0) so the verifier emits a zext instruction after the mov.
>
> Fixes: 6082b6c328b5 ("bpf: Recognize addr_space_cast instruction in the verifier.")
> Signed-off-by: Puranjay Mohan <puranjay12@...il.com>
> ---
>  kernel/bpf/verifier.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index de7813947981..ee796402ef60 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14046,8 +14046,11 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
>                                 if (insn->imm) {
>                                         /* off == BPF_ADDR_SPACE_CAST */
>                                         mark_reg_unknown(env, regs, insn->dst_reg);
> -                                       if (insn->imm == 1) /* cast from as(1) to as(0) */
> +                                       if (insn->imm == 1) { /* cast from as(1) to as(0) */
>                                                 dst_reg->type = PTR_TO_ARENA;
> +                                               /* PTR_TO_ARENA is 32-bit */
> +                                               dst_reg->subreg_def = env->insn_idx + 1;
> +                                       }
>                                 } else if (insn->off == 0) {
>                                         /* case: R1 = R2
>                                          * copy register state to dest reg
> @@ -19606,8 +19609,9 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>                             (((struct bpf_map *)env->prog->aux->arena)->map_flags & BPF_F_NO_USER_CONV)) {
>                                 /* convert to 32-bit mov that clears upper 32-bit */
>                                 insn->code = BPF_ALU | BPF_MOV | BPF_X;
> -                               /* clear off, so it's a normal 'wX = wY' from JIT pov */
> +                               /* clear off and imm, so it's a normal 'wX = wY' from JIT pov */
>                                 insn->off = 0;
> +                               insn->imm = 0;
>                         } /* cast from as(0) to as(1) should be handled by JIT */
>                         goto next_insn;
>                 }
> --
> 2.40.1
>

This did not reach the list somehow.
I will have to resend to trigger the CI.

Sorry for the noise.

Puranjay