Re: [PATCH bpf] bpf: verifier: fix addr_space_cast from as(1) to as(0)

[Puranjay Mohan]

On Thu, Mar 21, 2024 at 4:03 PM Puranjay Mohan <puranjay12@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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