[bpf-next] bpf: Fix verifier error due to narrower scalar spill onto 64-bit spilled scalar slots

Commit Message

Tao Lyu April 3, 2024, 8:24 p.m. UTC

The verifier previously aimed to reject partial overwrite on an 8-byte stack slot
that contains a spilled pointer.
However, it rejects all partial stack overwrites
as long as the targeted stack slot is a spilled register,
because it does not check if the stack slot is a spilled pointer.

Finally, incomplete checks will result in the rejection of valid programs,
which spill narrower scalar values onto scalar slots, as shown below.

0: R1=ctx() R10=fp0
; asm volatile ( @ repro.bpf.c:679
0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
1: (62) *(u32 *)(r10 -8) = 1
attempt to corrupt spilled pointer on stack
processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.

The issue is fixed by adding a check on the spilled register type of targeted slots.

Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
Signed-off-by: Tao Lyu <[e-mail]>
---
 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

Comments

John Fastabend April 3, 2024, 9:40 p.m. UTC | #1

Tao Lyu wrote:
> The verifier previously aimed to reject partial overwrite on an 8-byte stack slot
> that contains a spilled pointer.
> However, it rejects all partial stack overwrites
> as long as the targeted stack slot is a spilled register,
> because it does not check if the stack slot is a spilled pointer.
> 
> Finally, incomplete checks will result in the rejection of valid programs,
> which spill narrower scalar values onto scalar slots, as shown below.
> 
> 0: R1=ctx() R10=fp0
> ; asm volatile ( @ repro.bpf.c:679
> 0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
> 1: (62) *(u32 *)(r10 -8) = 1
> attempt to corrupt spilled pointer on stack
> processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.
> 
> The issue is fixed by adding a check on the spilled register type of targeted slots.
> 
> Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> Signed-off-by: Tao Lyu <[e-mail]>
> ---

A test case would be nice.

>  kernel/bpf/verifier.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 17c06f1505e4..3064ba7c140f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>  	 */
>  	if (!env->allow_ptr_leaks &&
>  	    is_spilled_reg(&state->stack[spi]) &&
> +	    state->stack[spi].spilled_ptr.type != SCALAR_VALUE &&
>  	    size != BPF_REG_SIZE) {

Might be worth a helper we have is_spilled_reg and is_spilled_scalar_reg
maybe a is_spilled_not_scalar_reg() is nice. Perhaps Andrii has a style
opinion. Or,

 is_spilled_reg(...) && !is_spilled_scalar_reg(...)

Not sure I like that any better than whats there in patch though.

>  		verbose(env, "attempt to corrupt spilled pointer on stack\n");
>  		return -EACCES;
> -- 
> 2.25.1
> 
>

Andrii Nakryiko April 3, 2024, 9:46 p.m. UTC | #2

On Wed, Apr 3, 2024 at 2:40 PM John Fastabend <[e-mail]> wrote:
>
> Tao Lyu wrote:
> > The verifier previously aimed to reject partial overwrite on an 8-byte stack slot
> > that contains a spilled pointer.
> > However, it rejects all partial stack overwrites
> > as long as the targeted stack slot is a spilled register,
> > because it does not check if the stack slot is a spilled pointer.
> >
> > Finally, incomplete checks will result in the rejection of valid programs,
> > which spill narrower scalar values onto scalar slots, as shown below.
> >
> > 0: R1=ctx() R10=fp0
> > ; asm volatile ( @ repro.bpf.c:679
> > 0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
> > 1: (62) *(u32 *)(r10 -8) = 1
> > attempt to corrupt spilled pointer on stack
> > processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.
> >
> > The issue is fixed by adding a check on the spilled register type of targeted slots.
> >
> > Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> > Signed-off-by: Tao Lyu <[e-mail]>
> > ---
>
> A test case would be nice.

+1, please see progs/verifier_spill_fill.c, seems like a logical place

>
> >  kernel/bpf/verifier.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 17c06f1505e4..3064ba7c140f 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> >        */
> >       if (!env->allow_ptr_leaks &&
> >           is_spilled_reg(&state->stack[spi]) &&
> > +         state->stack[spi].spilled_ptr.type != SCALAR_VALUE &&
> >           size != BPF_REG_SIZE) {
>
> Might be worth a helper we have is_spilled_reg and is_spilled_scalar_reg
> maybe a is_spilled_not_scalar_reg() is nice. Perhaps Andrii has a style
> opinion. Or,
>
>  is_spilled_reg(...) && !is_spilled_scalar_reg(...)

yeah, I'd probably go with this, definitely not "is_spilled_not_scalar_reg" :)

>
> Not sure I like that any better than whats there in patch though.
>
> >               verbose(env, "attempt to corrupt spilled pointer on stack\n");
> >               return -EACCES;
> > --
> > 2.25.1
> >
> >
>
>

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 17c06f1505e4..3064ba7c140f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4493,6 +4493,7 @@  static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
 	 */
 	if (!env->allow_ptr_leaks &&
 	    is_spilled_reg(&state->stack[spi]) &&
+	    state->stack[spi].spilled_ptr.type != SCALAR_VALUE &&
 	    size != BPF_REG_SIZE) {
 		verbose(env, "attempt to corrupt spilled pointer on stack\n");
 		return -EACCES;