check_stack_write_var_off() does not reject pointer reg, this can lead
to pointer leak. When cpu_mitigation_off(), unprivileged users can add
var off to stack pointer, and loading the following prog enable them
leak kernel address:
func#0 @0
0: R1=ctx() R10=fp0
0: (7a) *(u64 *)(r10 -8) = 0 ; R10=fp0 fp-8_w=00000000
1: (7a) *(u64 *)(r10 -16) = 0 ; R10=fp0 fp-16_w=00000000
2: (7a) *(u64 *)(r10 -24) = 0 ; R10=fp0 fp-24_w=00000000
3: (bf) r6 = r1 ; R1=ctx() R6_w=ctx()
4: (b7) r1 = 8 ; R1_w=P8
5: (37) r1 /= 1 ; R1_w=Pscalar()
6: (57) r1 &= 8 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
7: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
8: (07) r2 += -16 ; R2_w=fp-16
9: (0f) r2 += r1 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8)) R2_w=fp(off=-16,smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
10: (7b) *(u64 *)(r2 +0) = r6 ; R2_w=fp(off=-16,smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8)) R6_w=ctx() fp-8_w=mmmmmmmm fp-16_w=mmmmmmmm
11: (18) r1 = 0x0 ; R1_w=map_ptr(ks=4,vs=8)
13: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
14: (07) r2 += -16 ; R2_w=fp-16
15: (bf) r3 = r10 ; R3_w=fp0 R10=fp0
16: (07) r3 += -8 ; R3_w=fp-8
17: (b7) r4 = 0 ; R4_w=P0
18: (85) call bpf_map_update_elem#2 ; R0_w=Pscalar()
19: (79) r0 = *(u64 *)(r10 -8) ; R0_w=Pscalar() R10=fp0 fp-8_w=mmmmmmmm
20: (95) exit
processed 20 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
The prog first inits several slots, so it later can access, and then
adds var-off to fp, where it knows the off is -8. Finally, the prog
spills the ctx ptr and leaks it to a map, and unprivileged users can
read the pointer through a map lookup:
Leaked Map Address: 0xffff98d3828f5700
Fix this by rejecting pointer reg in check_stack_write_var_off().
Applying the patch makes the prog rejected with "spilling pointer
with var-offset is disallowed".
Also add missed newline to error messages in this check.
Signed-off-by: Hao Sun <[email protected]>
---
Note that it's hard to add this test to test_progs or test_verifier, as
this requires cpu_mitigation_off() setup, currently tested on my local.
kernel/bpf/verifier.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f31868ba0c2d..c34b938fa06f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4627,6 +4627,11 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
(!value_reg && is_bpf_st_mem(insn) && insn->imm == 0))
writing_zero = true;
+ if (value_reg && __is_pointer_value(env->allow_ptr_leaks, value_reg)) {
+ verbose(env, "spilling pointer with var-offset is disallowed\n");
+ return -EINVAL;
+ }
+
for (i = min_off; i < max_off; i++) {
int spi;
@@ -4658,7 +4663,7 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
* later for CAP_PERFMON, as the write may not happen to
* that slot.
*/
- verbose(env, "spilled ptr in range of var-offset stack write; insn %d, ptr off: %d",
+ verbose(env, "spilled ptr in range of var-offset stack write; insn %d, ptr off: %d\n",
insn_idx, i);
return -EINVAL;
}
@@ -4694,7 +4699,7 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
* them, the error would be too confusing.
*/
if (*stype == STACK_INVALID && !env->allow_uninit_stack) {
- verbose(env, "uninit stack in range of var-offset write prohibited for !root; insn %d, off: %d",
+ verbose(env, "uninit stack in range of var-offset write prohibited for !root; insn %d, off: %d\n",
insn_idx, i);
return -EINVAL;
}
--
2.34.1
On Wed, 2024-01-24 at 11:30 +0100, Hao Sun wrote:
> check_stack_write_var_off() does not reject pointer reg, this can lead
> to pointer leak. When cpu_mitigation_off(), unprivileged users can add
> var off to stack pointer, and loading the following prog enable them
> leak kernel address:
>
> func#0 @0
> 0: R1=ctx() R10=fp0
> 0: (7a) *(u64 *)(r10 -8) = 0 ; R10=fp0 fp-8_w=00000000
> 1: (7a) *(u64 *)(r10 -16) = 0 ; R10=fp0 fp-16_w=00000000
> 2: (7a) *(u64 *)(r10 -24) = 0 ; R10=fp0 fp-24_w=00000000
> 3: (bf) r6 = r1 ; R1=ctx() R6_w=ctx()
> 4: (b7) r1 = 8 ; R1_w=P8
> 5: (37) r1 /= 1 ; R1_w=Pscalar()
> 6: (57) r1 &= 8 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
> 7: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
> 8: (07) r2 += -16 ; R2_w=fp-16
> 9: (0f) r2 += r1 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8)) R2_w=fp(off=-16,smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
> 10: (7b) *(u64 *)(r2 +0) = r6 ; R2_w=fp(off=-16,smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8)) R6_w=ctx() fp-8_w=mmmmmmmm fp-16_w=mmmmmmmm
> 11: (18) r1 = 0x0 ; R1_w=map_ptr(ks=4,vs=8)
> 13: (bf) r2 = r10 ; R2_w=fp0 R10=fp0
> 14: (07) r2 += -16 ; R2_w=fp-16
> 15: (bf) r3 = r10 ; R3_w=fp0 R10=fp0
> 16: (07) r3 += -8 ; R3_w=fp-8
> 17: (b7) r4 = 0 ; R4_w=P0
> 18: (85) call bpf_map_update_elem#2 ; R0_w=Pscalar()
> 19: (79) r0 = *(u64 *)(r10 -8) ; R0_w=Pscalar() R10=fp0 fp-8_w=mmmmmmmm
> 20: (95) exit
> processed 20 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
I tried this example as a part of selftest
(If put to tools/testing/selftests/bpf/progs/verifier_map_ptr.c
could be executed using command:
./test_progs -vvv -a 'verifier_map_ptr/ctx_addr_leak @unpriv'):
SEC("socket")
__failure_unpriv
__msg_unpriv("spilling pointer with var-offset is disallowed")
__naked void ctx_addr_leak(void)
{
asm volatile (
"r0 = 0;"
"*(u64 *)(r10 -8) = r0;"
"*(u64 *)(r10 -16) = r0;"
"*(u64 *)(r10 -24) = r0;"
"r6 = r1;"
"r1 = 8;"
"r1 /= 1;"
"r1 &= 8;"
"r2 = r10;"
"r2 += -16;"
"r2 += r1;"
"*(u64 *)(r2 +0) = r6;"
"r1 = %[map_hash_16b] ll;"
"r2 = r10;"
"r2 += -16;"
"r3 = r10;"
"r3 += -8;"
"r4 = 0;"
"call %[bpf_map_update_elem];"
"r0 = *(u64 *)(r10 -8);"
"exit;"
:
: __imm(bpf_map_update_elem),
__imm_addr(map_hash_16b)
: __clobber_all);
}
And see the following error message:
..
r1 &= 8 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
r2 = r10 ; R2_w=fp0 R10=fp0
r2 += -16 ; R2_w=fp-16
r2 += r1
R2 variable stack access prohibited for !root, var_off=(0x0; 0x8) off=-16
Could you please craft a selftest that checks for expected message?
Overall the change makes sense to me.
>
> I tried this example as a part of selftest
> (If put to tools/testing/selftests/bpf/progs/verifier_map_ptr.c
> could be executed using command:
> ./test_progs -vvv -a 'verifier_map_ptr/ctx_addr_leak @unpriv'):
>
> SEC("socket")
> __failure_unpriv
> __msg_unpriv("spilling pointer with var-offset is disallowed")
> __naked void ctx_addr_leak(void)
> {
> asm volatile (
> "r0 = 0;"
> "*(u64 *)(r10 -8) = r0;"
> "*(u64 *)(r10 -16) = r0;"
> "*(u64 *)(r10 -24) = r0;"
> "r6 = r1;"
> "r1 = 8;"
> "r1 /= 1;"
> "r1 &= 8;"
> "r2 = r10;"
> "r2 += -16;"
> "r2 += r1;"
> "*(u64 *)(r2 +0) = r6;"
> "r1 = %[map_hash_16b] ll;"
> "r2 = r10;"
> "r2 += -16;"
> "r3 = r10;"
> "r3 += -8;"
> "r4 = 0;"
> "call %[bpf_map_update_elem];"
> "r0 = *(u64 *)(r10 -8);"
> "exit;"
> :
> : __imm(bpf_map_update_elem),
> __imm_addr(map_hash_16b)
> : __clobber_all);
> }
>
> And see the following error message:
>
> ...
> r1 &= 8 ; R1_w=Pscalar(smin=smin32=0,smax=umax=smax32=umax32=8,var_off=(0x0; 0x8))
> r2 = r10 ; R2_w=fp0 R10=fp0
> r2 += -16 ; R2_w=fp-16
> r2 += r1
> R2 variable stack access prohibited for !root, var_off=(0x0; 0x8) off=-16
>
> Could you please craft a selftest that checks for expected message?
> Overall the change makes sense to me.
Testing this case with test_progs/test_verifier is hard because it happens
when cpu_mitigations_off() is true, but we do not have this setup yet.
So the mentioned prog is rejected by sanitize_check_bounds() due to ptr
alu with var_off when adding it to test_progs, and loading as unpriv.
My local test was conducted: (1) booting the kernel with "mitigations=off"
so that bypass_spec_v1 is true and sanitize_check_bounds() is skipped;
(2) running the prog without the patch leaks the pointer; (3) loading the
prog with the patch applied resulting in the expected message.
On Thu, 2024-01-25 at 09:34 +0100, Hao Sun wrote:
[...]
> Testing this case with test_progs/test_verifier is hard because it happens
> when cpu_mitigations_off() is true, but we do not have this setup yet.
> So the mentioned prog is rejected by sanitize_check_bounds() due to ptr
> alu with var_off when adding it to test_progs, and loading as unpriv.
>
> My local test was conducted: (1) booting the kernel with "mitigations=off"
> so that bypass_spec_v1 is true and sanitize_check_bounds() is skipped;
> (2) running the prog without the patch leaks the pointer; (3) loading the
> prog with the patch applied resulting in the expected message.
Thank you for explaining.
I booted VM with "mitigations=off" and tried test as in [1], it passes.
Tested-by: Eduard Zingerman <[email protected]>
[1] https://gist.github.com/eddyz87/bb517437767a8f01891cc6e6a847d448