| Message ID | 20231204192601.2672497-4-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Complete BPF verifier precision tracking support for register spills | expand |
On Mon, 2023-12-04 at 11:25 -0800, Andrii Nakryiko wrote: [...] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 4f8a3c77eb80..73315e2f20d9 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, > * so it's aligned access and [off, off + size) are within stack limits > */ > if (!env->allow_ptr_leaks && > - state->stack[spi].slot_type[0] == STACK_SPILL && > + is_spilled_reg(&state->stack[spi]) && > size != BPF_REG_SIZE) { > verbose(env, "attempt to corrupt spilled pointer on stack\n"); > return -EACCES; I think there is a small detail here. slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit. Thus, with this patch applied the test below does not pass. Log fragment: 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) 2: (63) *(u32 *)(r10 -8) = r0 3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) 3: (b7) r0 = 42 ; R0_w=42 4: (63) *(u32 *)(r10 -4) = r0 attempt to corrupt spilled pointer on stack Admittedly, this happens only when the only capability is CAP_BPF and we don't test this configuration. --- iff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c index 359df865a8f3..61ada86e84df 100644 --- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c @@ -97,4 +97,20 @@ __naked void misaligned_read_from_stack(void) " ::: __clobber_all); } +SEC("socket") +__success_unpriv +__naked void spill_lo32_write_hi32(void) +{ + asm volatile (" \ + call %[bpf_get_prandom_u32]; \ + r0 &= 0xffff; \ + *(u32*)(r10 - 8) = r0; \ + r0 = 42; \ + *(u32*)(r10 - 4) = r0; \ + exit; \ +" : + : __imm(bpf_get_prandom_u32) + : __clobber_all); +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c index a350ecdfba4a..a5ad6b01175e 100644 --- a/tools/testing/selftests/bpf/test_loader.c +++ b/tools/testing/selftests/bpf/test_loader.c @@ -430,7 +430,7 @@ struct cap_state { static int drop_capabilities(struct cap_state *caps) { const __u64 caps_to_drop = (1ULL << CAP_SYS_ADMIN | 1ULL << CAP_NET_ADMIN | - 1ULL << CAP_PERFMON | 1ULL << CAP_BPF); + 1ULL << CAP_PERFMON /*| 1ULL << CAP_BPF */); int err; err = cap_disable_effective(caps_to_drop, &caps->old_caps);
On Tue, 2023-12-05 at 00:12 +0200, Eduard Zingerman wrote: [...] > diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c > index a350ecdfba4a..a5ad6b01175e 100644 > --- a/tools/testing/selftests/bpf/test_loader.c > +++ b/tools/testing/selftests/bpf/test_loader.c > @@ -430,7 +430,7 @@ struct cap_state { > static int drop_capabilities(struct cap_state *caps) > { > const __u64 caps_to_drop = (1ULL << CAP_SYS_ADMIN | 1ULL << CAP_NET_ADMIN | > - 1ULL << CAP_PERFMON | 1ULL << CAP_BPF); > + 1ULL << CAP_PERFMON /*| 1ULL << CAP_BPF */); > int err; > > err = cap_disable_effective(caps_to_drop, &caps->old_caps); (Here I hack test_loader so that unpriv run has CAP_BPF, the test could be run as follows: ./test_progs -vvv -a 'verifier_basic_stack/spill_lo32_write_hi32 @unpriv')
On Mon, Dec 4, 2023 at 2:12 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2023-12-04 at 11:25 -0800, Andrii Nakryiko wrote: > [...] > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 4f8a3c77eb80..73315e2f20d9 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, > > * so it's aligned access and [off, off + size) are within stack limits > > */ > > if (!env->allow_ptr_leaks && > > - state->stack[spi].slot_type[0] == STACK_SPILL && > > + is_spilled_reg(&state->stack[spi]) && > > size != BPF_REG_SIZE) { > > verbose(env, "attempt to corrupt spilled pointer on stack\n"); > > return -EACCES; > > I think there is a small detail here. > slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit. Hm... I wonder if the check was written like this deliberately to prevent turning any spilled register into STACK_MISC? > Thus, with this patch applied the test below does not pass. > Log fragment: > > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > 2: (63) *(u32 *)(r10 -8) = r0 > 3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > 3: (b7) r0 = 42 ; R0_w=42 > 4: (63) *(u32 *)(r10 -4) = r0 > attempt to corrupt spilled pointer on stack What would happen if we have 4: *(u16 *)(r10 - 8) = 123; ? and similarly 4: *(u16 *)(r10 - 6) = 123; ? (16-bit overwrites of spilled 32-bit register) It should be rejected, can you please quickly check if they will be with the existing check? So it makes me feel like the intent was to reject any partial writes with spilled reg slots. We could probably improve that to just make sure that we don't turn spilled pointers into STACK_MISC in unpriv, but I'm not sure if it's worth doing that instead of keeping things simple? Alexei, do you remember what was the original intent? > > Admittedly, this happens only when the only capability is CAP_BPF and > we don't test this configuration. > > --- > > iff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c > index 359df865a8f3..61ada86e84df 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c > +++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c > @@ -97,4 +97,20 @@ __naked void misaligned_read_from_stack(void) > " ::: __clobber_all); > } > > +SEC("socket") > +__success_unpriv > +__naked void spill_lo32_write_hi32(void) > +{ > + asm volatile (" \ > + call %[bpf_get_prandom_u32]; \ > + r0 &= 0xffff; \ > + *(u32*)(r10 - 8) = r0; \ > + r0 = 42; \ > + *(u32*)(r10 - 4) = r0; \ > + exit; \ > +" : > + : __imm(bpf_get_prandom_u32) > + : __clobber_all); > +} > + > char _license[] SEC("license") = "GPL"; > diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c > index a350ecdfba4a..a5ad6b01175e 100644 > --- a/tools/testing/selftests/bpf/test_loader.c > +++ b/tools/testing/selftests/bpf/test_loader.c > @@ -430,7 +430,7 @@ struct cap_state { > static int drop_capabilities(struct cap_state *caps) > { > const __u64 caps_to_drop = (1ULL << CAP_SYS_ADMIN | 1ULL << CAP_NET_ADMIN | > - 1ULL << CAP_PERFMON | 1ULL << CAP_BPF); > + 1ULL << CAP_PERFMON /*| 1ULL << CAP_BPF */); > int err; > > err = cap_disable_effective(caps_to_drop, &caps->old_caps);
On Mon, 2023-12-04 at 16:23 -0800, Andrii Nakryiko wrote: [...] > > > @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, > > > * so it's aligned access and [off, off + size) are within stack limits > > > */ > > > if (!env->allow_ptr_leaks && > > > - state->stack[spi].slot_type[0] == STACK_SPILL && > > > + is_spilled_reg(&state->stack[spi]) && > > > size != BPF_REG_SIZE) { > > > verbose(env, "attempt to corrupt spilled pointer on stack\n"); > > > return -EACCES; > > > > I think there is a small detail here. > > slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit. > > Hm... I wonder if the check was written like this deliberately to > prevent turning any spilled register into STACK_MISC? idk, the error is about pointers and forbidding turning pointers to STACK_MISC makes sense. Don't see why it would be useful to forbid this for scalars. > > Thus, with this patch applied the test below does not pass. > > Log fragment: > > > > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > > 2: (63) *(u32 *)(r10 -8) = r0 > > 3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > > 3: (b7) r0 = 42 ; R0_w=42 > > 4: (63) *(u32 *)(r10 -4) = r0 > > attempt to corrupt spilled pointer on stack > > What would happen if we have > > 4: *(u16 *)(r10 - 8) = 123; ? w/o this patch: 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) 3: (b7) r0 = 123 ; R0_w=123 4: (6b) *(u16 *)(r10 -8) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmm123 5: (95) exit with this patch: 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) 3: (b7) r0 = 123 ; R0_w=123 4: (6b) *(u16 *)(r10 -8) = r0 attempt to corrupt spilled pointer on stack > and similarly > > 4: *(u16 *)(r10 - 6) = 123; ? w/o this patch: 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(....,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) 3: (b7) r0 = 123 ; R0_w=123 4: (6b) *(u16 *)(r10 -6) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmmmm 5: (95) exit with this patch: 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) 3: (b7) r0 = 123 ; R0_w=123 4: (6b) *(u16 *)(r10 -6) = r0 attempt to corrupt spilled pointer on stack > So it makes me feel like the intent was to reject any partial writes > with spilled reg slots. We could probably improve that to just make > sure that we don't turn spilled pointers into STACK_MISC in unpriv, > but I'm not sure if it's worth doing that instead of keeping things > simple? You mean like below? if (!env->allow_ptr_leaks && is_spilled_reg(&state->stack[spi]) && is_spillable_regtype(state->stack[spi].spilled_ptr.type) && size != BPF_REG_SIZE) { verbose(env, "attempt to corrupt spilled pointer on stack\n"); return -EACCES; }
On Mon, Dec 4, 2023 at 4:23 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > Alexei, do you remember what was the original intent? Commit 27113c59b6d0 ("bpf: Check the other end of slot_type for STACK_SPILL") introduced is_spilled_reg() and at that time it tried to convert all slot_type[0] to slot_type[7] checks. Looks like this one was simply missed. The fixes tag you have: Fixes: 638f5b90d460 ("bpf: reduce verifier memory consumption") is much older than the introduction of is_spilled_reg. At that time everything was checking slot_type[0]. So this fixes tag is somewhat wrong. Probably Fixes: 27113c59b6d0 would be more correct.
On Mon, Dec 4, 2023 at 5:45 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Dec 4, 2023 at 4:23 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > Alexei, do you remember what was the original intent? > > Commit 27113c59b6d0 ("bpf: Check the other end of slot_type for STACK_SPILL") > introduced is_spilled_reg() and at that time it tried to convert > all slot_type[0] to slot_type[7] checks. > > Looks like this one was simply missed. ok, so this seems like a correct fix, at least according to original intent, great > > The fixes tag you have: > Fixes: 638f5b90d460 ("bpf: reduce verifier memory consumption") > is much older than the introduction of is_spilled_reg. > At that time everything was checking slot_type[0]. > So this fixes tag is somewhat wrong. > Probably Fixes: 27113c59b6d0 would be more correct. yep, will use that, thanks.
On Mon, Dec 4, 2023 at 4:54 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2023-12-04 at 16:23 -0800, Andrii Nakryiko wrote: > [...] > > > > @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, > > > > * so it's aligned access and [off, off + size) are within stack limits > > > > */ > > > > if (!env->allow_ptr_leaks && > > > > - state->stack[spi].slot_type[0] == STACK_SPILL && > > > > + is_spilled_reg(&state->stack[spi]) && > > > > size != BPF_REG_SIZE) { > > > > verbose(env, "attempt to corrupt spilled pointer on stack\n"); > > > > return -EACCES; > > > > > > I think there is a small detail here. > > > slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit. > > > > Hm... I wonder if the check was written like this deliberately to > > prevent turning any spilled register into STACK_MISC? > > idk, the error is about pointers and forbidding turning pointers to > STACK_MISC makes sense. Don't see why it would be useful to forbid > this for scalars. you are correct that this check doesn't make sense for SCALAR_VALUE register spill, I think the intent was to prevent pointer spills. But that's an orthogonal issue, this could be improved separately. > > > > Thus, with this patch applied the test below does not pass. > > > Log fragment: > > > > > > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > > > 2: (63) *(u32 *)(r10 -8) = r0 > > > 3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > > > 3: (b7) r0 = 42 ; R0_w=42 > > > 4: (63) *(u32 *)(r10 -4) = r0 > > > attempt to corrupt spilled pointer on stack > > > > What would happen if we have > > > > 4: *(u16 *)(r10 - 8) = 123; ? > > w/o this patch: > > 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > 3: (b7) r0 = 123 ; R0_w=123 > 4: (6b) *(u16 *)(r10 -8) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmm123 > 5: (95) exit > > with this patch: > > 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > 3: (b7) r0 = 123 ; R0_w=123 > 4: (6b) *(u16 *)(r10 -8) = r0 > attempt to corrupt spilled pointer on stack ok, so SCALAR_VALUE aside, if it was some pointer, we should be rejecting these writes > > > and similarly > > > > 4: *(u16 *)(r10 - 6) = 123; ? > > w/o this patch: > > 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(....,var_off=(0x0; 0xffff)) > R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > 3: (b7) r0 = 123 ; R0_w=123 > 4: (6b) *(u16 *)(r10 -6) = r0 ; R0_w=123 R10=fp0 fp-8=mmmmmmmm > 5: (95) exit > > with this patch: > > 0: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar() > 1: (57) r0 &= 65535 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > 2: (63) *(u32 *)(r10 -8) = r0 ; R0_w=scalar(...,var_off=(0x0; 0xffff)) > R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff)) > 3: (b7) r0 = 123 ; R0_w=123 > 4: (6b) *(u16 *)(r10 -6) = r0 > attempt to corrupt spilled pointer on stack > > > So it makes me feel like the intent was to reject any partial writes > > with spilled reg slots. We could probably improve that to just make > > sure that we don't turn spilled pointers into STACK_MISC in unpriv, > > but I'm not sure if it's worth doing that instead of keeping things > > simple? > > You mean like below? > > if (!env->allow_ptr_leaks && > is_spilled_reg(&state->stack[spi]) && > is_spillable_regtype(state->stack[spi].spilled_ptr.type) && Honestly, I wouldn't trust is_spillable_regtype() the way it's written, it's too easy to forget to add a new register type to the list. I think the only "safe to spill" register is probably SCALAR_VALUE, so I'd just do `type != SCALAR_VALUE`. But yes, I think that's the right approach. If we were being pedantic, though, we'd need to take into account offset and see if [offset, offset + size) overlaps with any STACK_SPILL/STACK_DYNPTR/STACK_ITER slots. But tbh, given it's unpriv programs we are talking about, I probably wouldn't bother extending this logic too much. > size != BPF_REG_SIZE) { > verbose(env, "attempt to corrupt spilled pointer on stack\n"); > return -EACCES; > }
On Mon, 2023-12-04 at 19:56 -0800, Andrii Nakryiko wrote: [...] > > > So it makes me feel like the intent was to reject any partial writes > > > with spilled reg slots. We could probably improve that to just make > > > sure that we don't turn spilled pointers into STACK_MISC in unpriv, > > > but I'm not sure if it's worth doing that instead of keeping things > > > simple? > > > > You mean like below? > > > > if (!env->allow_ptr_leaks && > > is_spilled_reg(&state->stack[spi]) && > > is_spillable_regtype(state->stack[spi].spilled_ptr.type) && > > Honestly, I wouldn't trust is_spillable_regtype() the way it's > written, it's too easy to forget to add a new register type to the > list. I think the only "safe to spill" register is probably > SCALAR_VALUE, so I'd just do `type != SCALAR_VALUE`. > > But yes, I think that's the right approach. 'type != SCALAR_VALUE' makes sense as well. Do you plan to add this check as a part of current patch? > If we were being pedantic, though, we'd need to take into account > offset and see if [offset, offset + size) overlaps with any > STACK_SPILL/STACK_DYNPTR/STACK_ITER slots. > > But tbh, given it's unpriv programs we are talking about, I probably > wouldn't bother extending this logic too much. Yes, that's definitely is an ommission.
On Tue, Dec 5, 2023 at 5:34 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2023-12-04 at 19:56 -0800, Andrii Nakryiko wrote: > [...] > > > > So it makes me feel like the intent was to reject any partial writes > > > > with spilled reg slots. We could probably improve that to just make > > > > sure that we don't turn spilled pointers into STACK_MISC in unpriv, > > > > but I'm not sure if it's worth doing that instead of keeping things > > > > simple? > > > > > > You mean like below? > > > > > > if (!env->allow_ptr_leaks && > > > is_spilled_reg(&state->stack[spi]) && > > > is_spillable_regtype(state->stack[spi].spilled_ptr.type) && > > > > Honestly, I wouldn't trust is_spillable_regtype() the way it's > > written, it's too easy to forget to add a new register type to the > > list. I think the only "safe to spill" register is probably > > SCALAR_VALUE, so I'd just do `type != SCALAR_VALUE`. > > > > But yes, I think that's the right approach. > > 'type != SCALAR_VALUE' makes sense as well. > Do you plan to add this check as a part of current patch? nope :) this will turn into another retval patch set story. Feel free to follow up if you care enough about this, though! > > > If we were being pedantic, though, we'd need to take into account > > offset and see if [offset, offset + size) overlaps with any > > STACK_SPILL/STACK_DYNPTR/STACK_ITER slots. > > > > But tbh, given it's unpriv programs we are talking about, I probably > > wouldn't bother extending this logic too much. > > Yes, that's definitely is an ommission.
On Tue, 2023-12-05 at 10:30 -0800, Andrii Nakryiko wrote: [...] > > 'type != SCALAR_VALUE' makes sense as well. > > Do you plan to add this check as a part of current patch? > > nope :) this will turn into another retval patch set story. Feel free > to follow up if you care enough about this, though! Well, it's a regression. On the other hand at my old job we considered that feature does not exist if it's not covered by a test. I'll do a follow-up.
On Tue, Dec 5, 2023 at 10:49 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2023-12-05 at 10:30 -0800, Andrii Nakryiko wrote: > [...] > > > 'type != SCALAR_VALUE' makes sense as well. > > > Do you plan to add this check as a part of current patch? > > > > nope :) this will turn into another retval patch set story. Feel free > > to follow up if you care enough about this, though! > > Well, it's a regression. On the other hand at my old job we considered technically, but it was never meant to work, which is why I'm ok with fixing it by tightening the check > that feature does not exist if it's not covered by a test. > I'll do a follow-up. thanks!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 4f8a3c77eb80..73315e2f20d9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, * so it's aligned access and [off, off + size) are within stack limits */ if (!env->allow_ptr_leaks && - state->stack[spi].slot_type[0] == STACK_SPILL && + is_spilled_reg(&state->stack[spi]) && size != BPF_REG_SIZE) { verbose(env, "attempt to corrupt spilled pointer on stack\n"); return -EACCES;
When register is spilled onto a stack as a 1/2/4-byte register, we set slot_type[BPF_REG_SIZE - 1] (plus potentially few more below it, depending on actual spill size). So to check if some stack slot has spilled register we need to consult slot_type[7], not slot_type[0]. To avoid the need to remember and double-check this in the future, just use is_spilled_reg() helper. Fixes: 638f5b90d460 ("bpf: reduce verifier memory consumption") Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/bpf/verifier.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)