Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <20231011-jmp-into-reserved-fields-v3-0-97d2aa979788@gmail.com>
 <20231011-jmp-into-reserved-fields-v3-1-97d2aa979788@gmail.com>
 <CAADnVQJnhfbALtNkCauS_ZwRfybcb_mryEvZW7Uu1uOSshQ9Ew@mail.gmail.com>
 <ZSeq7ieG7Cq13w67@u94a> <CAADnVQJHAPid9HouwMEnfwDDKuy8BnGia269KSbby2gA030OBg@mail.gmail.com>
 <ZSi5PHDfoAYcvbCq@u94a>
In-Reply-To: <ZSi5PHDfoAYcvbCq@u94a>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Thu, 19 Oct 2023 17:25:26 -0700
Message-ID: <CAADnVQLiWk5_Wf3q6iDAyLb-n0W5je3Z8XT2J-mtZ5s9RA-JjQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v3 1/3] bpf: Detect jumping to reserved code
 during check_cfg()
To:     Shung-Hsi Yu <shung-hsi.yu@suse.com>
Cc:     Hao Sun <sunhao.th@gmail.com>, 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>,
        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 <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 12, 2023 at 8:28=E2=80=AFPM Shung-Hsi Yu <shung-hsi.yu@suse.com=
> wrote:
>
> On Thu, Oct 12, 2023 at 08:02:00AM -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 12, 2023 at 1:14=E2=80=AFAM Shung-Hsi Yu <shung-hsi.yu@suse=
.com> wrote:
> > > On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote:
> > > > On Wed, Oct 11, 2023 at 2:01=E2=80=AFAM Hao Sun <sunhao.th@gmail.co=
m> wrote:
> > > > >
> > > > > Currently, we don't check if the branch-taken of a jump is reserv=
ed code of
> > > > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). Th=
e verifier
> > > > > gives the following log in such case:
> > > > >
> > > > > func#0 @0
> > > > > 0: R1=3Dctx(off=3D0,imm=3D0) R10=3Dfp0
> > > > > 0: (18) r4 =3D 0xffff888103436000       ; R4_w=3Dmap_ptr(off=3D0,=
ks=3D4,vs=3D128,imm=3D0)
> > > > > 2: (18) r1 =3D 0x1d                     ; R1_w=3D29
> > > > > 4: (55) if r4 !=3D 0x0 goto pc+4        ; R4_w=3Dmap_ptr(off=3D0,=
ks=3D4,vs=3D128,imm=3D0)
> > > > > 5: (1c) w1 -=3D w1                      ; R1_w=3D0
> > > > > 6: (18) r5 =3D 0x32                     ; R5_w=3D50
> > > > > 8: (56) if w5 !=3D 0xfffffff4 goto pc-2
> > > > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1
> > > > > mark_precise: frame0: regs=3Dr5 stack=3D before 6: (18) r5 =3D 0x=
32
> > > > > 7: R5_w=3D50
> > > > > 7: BUG_ld_00
> > > > > invalid BPF_LD_IMM insn
> > > > >
> > > > > Here the verifier rejects the program because it thinks insn at 7=
 is an
> > > > > invalid BPF_LD_IMM, but such a error log is not accurate since th=
e issue
> > > > > is jumping to reserved code not because the program contains inva=
lid insn.
> > > > > Therefore, make the verifier check the jump target during check_c=
fg(). For
> > > > > the same program, the verifier reports the following log:
> > > > >
> > > > > func#0 @0
> > > > > jump to reserved code from insn 8 to 7
> > > > >
> > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> > > > > ---
> > > > >  kernel/bpf/verifier.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > > index eed7350e15f4..725ac0b464cf 100644
> > > > > --- a/kernel/bpf/verifier.c
> > > > > +++ b/kernel/bpf/verifier.c
> > > > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e,=
 struct bpf_verifier_env *env,
> > > > >  {
> > > > >         int *insn_stack =3D env->cfg.insn_stack;
> > > > >         int *insn_state =3D env->cfg.insn_state;
> > > > > +       struct bpf_insn *insns =3D env->prog->insnsi;
> > > > >
> > > > >         if (e =3D=3D FALLTHROUGH && insn_state[t] >=3D (DISCOVERE=
D | FALLTHROUGH))
> > > > >                 return DONE_EXPLORING;
> > > > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e=
, struct bpf_verifier_env *env,
> > > > >                 return -EINVAL;
> > > > >         }
> > > > >
> > > > > +       if (e =3D=3D BRANCH && insns[w].code =3D=3D 0) {
> > > > > +               verbose_linfo(env, t, "%d", t);
> > > > > +               verbose(env, "jump to reserved code from insn %d =
to %d\n", t, w);
> > > > > +               return -EINVAL;
> > > > > +       }
> > > >
> > > > I don't think we should be changing the verifier to make
> > > > fuzzer logs more readable.
> > >
> > > Taking fuzzer out of consideration, giving users clearer explanation =
for
> > > such verifier rejection could save a lot of head scratching.
> >
> > Users won't see such errors unless they are actively doing what
> > is not recommended.
> >
> > > Compiler shouldn't generate such program, but its plausible to forget=
 to
> > > account that BPF_LD_IMM64 consists of two instructions when writing
> > > assembly (especially with filter.h-like macros) and have it jump to t=
he 2nd
> > > part of BPF_LD_IMM64.
> >
> > Using macros to write bpf asm code is highly discouraged.
> > All kinds of errors are possible.
> > Bogus jump is just one of such mistakes.
> > Use naked functions and inline asm in C code that
> > both GCC and clang understand then you won't see bad jumps.
> > See selftets/bpf/verifier_*.c as an example.
>
> Understood, thanks for the explanation!
>
> Found them under progs/verifier_*.c inside the bpf selftest directory.
>
> > > > Same with patch 2. The code is fine as-is.
> > >
> > > The only way BPF_SIZE(insn->code) !=3D BPF_DW conditional in check_ld=
_imm()
> > > can be met right now is when we have a jump to the 2nd part of LD_IMM=
64; but
> > > what this conditional actually guard against is not straight-forward =
and
> > > quite confusing[1].
> >
> > There are plenty of cases in the verifier where we print
> > an error message. Some of them should be impossible due
> > to prior checks. In such cases we don't yell "verifier bug"
> > and are not going to do that in this case either.
>
> I agree, without patch 1 applied, the change to "verfier bug" in patch 2
> doesn't make sense and is just wrong. The point I'm trying to make is tha=
t
> the checks done by verifier are generally clear, you can make sense of wh=
y
> certain check are in place just by looking at the code, but
> BPF_SIZE(insn->code) !=3D BPF_DW is _not_ one of them.
>
> I got confused, (reading between the lines I believe) this had Hao puzzle=
d,
> and even Yongsong had to look twice[1] back then; so this check is certai=
nly
> not on-par with others we have in the verifier in terms of clarity, which
> leads to patches here as well as mine a while back.
>
> Perhaps we could reconsider making it more obvious how verifier prevents
> jump to reserved code/2nd instruction of LD_IMM64?

I agree that the message is confusing.
My point is that people see it only when they code in asm with macros.
Anyone who was doing that a lot saw that message and probably debugged
much worse issues while inserting an asm macro and forgetting to
adjust constants in branches. The code might even load, but will
execute something totally different.
asm macros are a nightmare to debug. Adding more code to the verifier
to help with one particular case is not going to help much.
Use inline asm in C is the right answer for folks that still need asm.

UX of the verifier sucks and we need to improve. So please focus on impactf=
ul
improvements instead of hacking on niche cases.