On Fri, Sep 1, 2023 at 7:57 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote:
>
> On Fri, Sep 01 2023, Puranjay Mohan wrote:
>
> > The problem here is that reg->subreg_def should be set as DEF_NOT_SUBREG for
> > registers that are used as destination registers of BPF_LDX |
> > BPF_MEMSX. I am seeing
> > the same problem on ARM32 and was going to send a patch today.
> >
> > The problem is that is_reg64() returns false for destination registers
> > of BPF_LDX | BPF_MEMSX.
> > But BPF_LDX | BPF_MEMSX always loads a 64 bit value because of the
> > sign extension so
> > is_reg64() should return true.
> >
> > I have written a patch that I will be sending as a reply to this.
> > Please let me know if that makes sense.
> >
>
> The check_reg_arg() function will mark reg->subreg_def = DEF_NOT_SUBREG for destination
> registers if is_reg64() returns true for these registers. My patch below make is_reg64()
> return true for destination registers of BPF_LDX with mod = BPF_MEMSX. I feel this is the
> correct way to fix this problem.
>
> Here is my patch:
>
> --- 8< ---
> From cf1bf5282183cf721926ab14d968d3d4097b89b8 Mon Sep 17 00:00:00 2001
> From: Puranjay Mohan <puranjay12@xxxxxxxxx>
> Date: Fri, 1 Sep 2023 11:18:59 +0000
> Subject: [PATCH bpf] bpf: verifier: mark destination of sign-extended load as
> 64 bit
>
> The verifier can emit instructions to zero-extend destination registers
> when the register is being used to keep 32 bit values. This behaviour is
> enabled only when the JIT sets bpf_jit_needs_zext() -> true. In the case
> of a sign extended load instruction, the destination register always has a
> 64-bit value, therefore the verifier should not emit zero-extend
> instructions for it.
>
> Change is_reg64() to return true if the register under consideration is a
> destination register of LDX instruction with mode = BPF_MEMSX.
>
> Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")
> Signed-off-by: Puranjay Mohan <puranjay12@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bb78212fa5b2..93f84b868ccc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3029,7 +3029,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>
> if (class == BPF_LDX) {
> if (t != SRC_OP)
> - return BPF_SIZE(code) == BPF_DW;
> + return (BPF_SIZE(code) == BPF_DW || BPF_MODE(code) == BPF_MEMSX);
Looks like we have a bug here for normal LDX too.
This 'if' condition was inserting unnecessary zext for LDX.
It was harmless for LDX and broken for LDSX.
Both LDX and LDSX write all bits of 64-bit register.
I think the proper fix is to remove above two lines.
wdyt?
