[PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[Andrii Nakryiko]

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@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 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 */
 }
 
 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)")
-- 
2.34.1

Re: [PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[Yonghong Song]

On 12/15/23 3:58 PM, 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@gmail.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[Maxim Mikityanskiy]

On Fri, 15 Dec 2023 at 15:58:22 -0800, 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@gmail.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  kernel/bpf/verifier.c                            | 16 +++-------------
>  .../selftests/bpf/progs/verifier_spill_fill.c    | 10 ++++++++--
>  2 files changed, 11 insertions(+), 15 deletions(-)

Thanks for the prompt fix!

Acked-by: Maxim Mikityanskiy <maxtram95@gmail.com>

Re: [PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[Daniel Borkmann]

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@gmail.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>   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().

>   }
>   
>   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)")
> 

Re: [PATCH bpf-next] bpf: ensure precise is reset to false in __mark_reg_const_zero()

[Andrii Nakryiko]

On Mon, Dec 18, 2023 at 2:46 AM Daniel Borkmann <daniel@iogearbox.net> 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@gmail.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   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)")
> >
>