Received-SPF: pass (google.com: domain of linux-kernel+bounces-38181-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240124103010.51408-1-sunhao.th@gmail.com> <5d33819c5f752755614882e30d971488731d97e0.camel@gmail.com>
In-Reply-To: <5d33819c5f752755614882e30d971488731d97e0.camel@gmail.com>
From: Hao Sun <sunhao.th@gmail.com>
Date: Thu, 25 Jan 2024 09:34:44 +0100
Message-ID: <CACkBjsZjYewSh4ZHFbj-D_Z7kGOeaVLfROcEDE1beNEDn-aU-A@mail.gmail.com>
Subject: Re: [PATCH bpf] bpf: Reject pointer spill with var offset
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: bpf@vger.kernel.org, andreimatei1@gmail.com, ast@kernel.org, 
	andrii@kernel.org, daniel@iogearbox.net, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

>
> 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.