Received-SPF: pass (google.com: domain of linux-kernel+bounces-110261-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240321150300.66598-1-puranjay12@gmail.com>
In-Reply-To: <20240321150300.66598-1-puranjay12@gmail.com>
From: Puranjay Mohan <puranjay12@gmail.com>
Date: Thu, 21 Mar 2024 16:38:26 +0100
Message-ID: <CANk7y0jJN7D8Df0b9-6TSvpwqMuQFyf8mmtyRBQ6hNtKFU3mvQ@mail.gmail.com>
Subject: Re: [PATCH bpf] bpf: verifier: fix addr_space_cast from as(1) to as(0)
To: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	John Fastabend <john.fastabend@gmail.com>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, 
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 21, 2024 at 4:03=E2=80=AFPM Puranjay Mohan <puranjay12@gmail.co=
m> wrote:
>
> The verifier currently converts addr_space_cast from as(1) to as(0) that
> is: BPF_ALU64 | BPF_MOV | BPF_X with off=3D1 and imm=3D1
> to
> BPF_ALU | BPF_MOV | BPF_X with imm=3D1 (32-bit mov)
>
> Because of this imm=3D1, the JITs that have bpf_jit_needs_zext() =3D=3D t=
rue,
> 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=3D0 when converting addr_space_cast to a
> normal mov32.
>
> The JITs that have bpf_jit_needs_zext() =3D=3D 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 v=
erifier.")
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.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 =3D=3D BPF_ADDR_SPACE_CAST=
 */
>                                         mark_reg_unknown(env, regs, insn-=
>dst_reg);
> -                                       if (insn->imm =3D=3D 1) /* cast f=
rom as(1) to as(0) */
> +                                       if (insn->imm =3D=3D 1) { /* cast=
 from as(1) to as(0) */
>                                                 dst_reg->type =3D PTR_TO_=
ARENA;
> +                                               /* PTR_TO_ARENA is 32-bit=
 */
> +                                               dst_reg->subreg_def =3D e=
nv->insn_idx + 1;
> +                                       }
>                                 } else if (insn->off =3D=3D 0) {
>                                         /* case: R1 =3D R2
>                                          * copy register state to dest re=
g
> @@ -19606,8 +19609,9 @@ static int do_misc_fixups(struct bpf_verifier_env=
 *env)
>                             (((struct bpf_map *)env->prog->aux->arena)->m=
ap_flags & BPF_F_NO_USER_CONV)) {
>                                 /* convert to 32-bit mov that clears uppe=
r 32-bit */
>                                 insn->code =3D BPF_ALU | BPF_MOV | BPF_X;
> -                               /* clear off, so it's a normal 'wX =3D wY=
' from JIT pov */
> +                               /* clear off and imm, so it's a normal 'w=
X =3D wY' from JIT pov */
>                                 insn->off =3D 0;
> +                               insn->imm =3D 0;
>                         } /* cast from as(0) to as(1) should be handled b=
y 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