On Mon, Dec 18, 2023 at 2:46 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 12/16/23 12:58 AM, Andrii Nakryiko wrote:
> > It is safe to always start with imprecise SCALAR_VALUE register.
> > Previously __mark_reg_const_zero() relied on caller to reset precise
> > mark, but it's very error prone and we already missed it in a few
> > places. So instead make __mark_reg_const_zero() reset precision always,
> > as it's a safe default for SCALAR_VALUE. Explanation is basically the
> > same as for why we are resetting (or rather not setting) precision in
> > current state. If necessary, precision propagation will set it to
> > precise correctly.
> >
> > As such, also remove a big comment about forward precision propagation
> > in mark_reg_stack_read() and avoid unnecessarily setting precision to
> > true after reading from STACK_ZERO stack. Again, precision propagation
> > will correctly handle this, if that SCALAR_VALUE register will ever be
> > needed to be precise.
> >
> > Reported-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 16 +++-------------
> > .../selftests/bpf/progs/verifier_spill_fill.c | 10 ++++++++--
> > 2 files changed, 11 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 1863826a4ac3..3009d1faec86 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -1781,6 +1781,7 @@ static void __mark_reg_const_zero(struct bpf_reg_state *reg)
> > {
> > __mark_reg_known(reg, 0);
> > reg->type = SCALAR_VALUE;
> > + reg->precise = false; /* all scalars are assumed imprecise initially */
>
> Could you elaborate on why it is safe to set it to false instead of using:
>
> reg->precise = !env->bpf_capable;
>
> For !cap_bpf we typically always set precise requirement to true, see also
> __mark_reg_unknown().
Oh, you are right, I forgot about unpriv. I'll send v2 taking unpriv
into account, thanks!
Let's also try this new fancy thing:
pw-bot: cr
>
> > }
> >
> > static void mark_reg_known_zero(struct bpf_verifier_env *env,
> > @@ -4706,21 +4707,10 @@ static void mark_reg_stack_read(struct bpf_verifier_env *env,
> > zeros++;
> > }
> > if (zeros == max_off - min_off) {
> > - /* any access_size read into register is zero extended,
> > - * so the whole register == const_zero
> > + /* Any access_size read into register is zero extended,
> > + * so the whole register == const_zero.
> > */
> > __mark_reg_const_zero(&state->regs[dst_regno]);
> > - /* backtracking doesn't support STACK_ZERO yet,
> > - * so mark it precise here, so that later
> > - * backtracking can stop here.
> > - * Backtracking may not need this if this register
> > - * doesn't participate in pointer adjustment.
> > - * Forward propagation of precise flag is not
> > - * necessary either. This mark is only to stop
> > - * backtracking. Any register that contributed
> > - * to const 0 was marked precise before spill.
> > - */
> > - state->regs[dst_regno].precise = true;
> > } else {
> > /* have read misc data from the stack */
> > mark_reg_unknown(env, state->regs, dst_regno);
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > index 508f5d6c7347..39fe3372e0e0 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > @@ -499,8 +499,14 @@ __success
> > __msg("2: (7a) *(u64 *)(r10 -8) = 0 ; R10=fp0 fp-8_w=00000000")
> > /* but fp-16 is spilled IMPRECISE zero const reg */
> > __msg("4: (7b) *(u64 *)(r10 -16) = r0 ; R0_w=0 R10=fp0 fp-16_w=0")
> > -/* and now check that precision propagation works even for such tricky case */
> > -__msg("10: (71) r2 = *(u8 *)(r10 -9) ; R2_w=P0 R10=fp0 fp-16_w=0")
> > +/* validate that assigning R2 from STACK_ZERO doesn't mark register
> > + * precise immediately; if necessary, it will be marked precise later
> > + */
> > +__msg("6: (71) r2 = *(u8 *)(r10 -1) ; R2_w=0 R10=fp0 fp-8_w=00000000")
> > +/* similarly, when R2 is assigned from spilled register, it is initially
> > + * imprecise, but will be marked precise later once it is used in precise context
> > + */
> > +__msg("10: (71) r2 = *(u8 *)(r10 -9) ; R2_w=0 R10=fp0 fp-16_w=0")
> > __msg("11: (0f) r1 += r2")
> > __msg("mark_precise: frame0: last_idx 11 first_idx 0 subseq_idx -1")
> > __msg("mark_precise: frame0: regs=r2 stack= before 10: (71) r2 = *(u8 *)(r10 -9)")
> >
>
