Re: [PATCH bpf] bpf: Reject pointer spill with var offset

[Eduard Zingerman]

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.