bpf: misleading spec_v1 check on variable-offset stack read?

bpf: misleading spec_v1 check on variable-offset stack read?

[Luis Gerhorst]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

Hello,

is there any way to introduce variable-offset (stack) pointers without 
using pointer arithmetic (BPF_ADD/SUB)? If yes, I believe this is a 
security issue because these can be used in stack writes. If not, I 
think the patch sent in reply to this mail should be applied. (I was not 
able to find any indication that the former is the case.)

Best regards,
Luis


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5976 bytes --]

[PATCH] bpf: remove misleading spec_v1 check on var-offset stack read

[Luis Gerhorst]

For every BPF_ADD/SUB involving a pointer, adjust_ptr_min_max_vals()
ensures that the resulting pointer has a constant offset if
bypass_spec_v1 is false. This is ensured by calling
sanitize_check_bounds() which in turn calls
check_stack_access_for_ptr_arithmetic(). There, -EACCESS is returned if
the register's offset is not constant, thereby rejecting the program.

In summary, an unprivileged user must never be able to create stack
pointers with a variable offset. That is also the case, because a
respective check in check_stack_write() is missing. If they were able to
create a variable-offset pointer, users could still use it in a
stack-write operation to trigger unsafe speculative behavior [1].

Because unprivileged users must already be prevented from creating
variable-offset stack pointers, viable options are to either remove this
check (replacing it with a clarifying comment), or to turn it into a
"verifier BUG"-message, also adding a similar check in
check_stack_write() (for consistency, as a second-level defense). This
patch implements the first option to reduce verifier bloat.

This check was introduced by commit 01f810ace9ed ("bpf: Allow
variable-offset stack access") which correctly notes that
"variable-offset reads and writes are disallowed (they were already
disallowed for the indirect access case) because the speculative
execution checking code doesn't support them". However, it does not
further discuss why the check in check_stack_read() is necessary. The
code which made this check obsolete was also introduced in this commit.

I have compiled ~650 programs from the Linux selftests, Linux samples,
Cilium, and libbpf/examples projects and confirmed that none of these
trigger the check in check_stack_read() [2]. Instead, all of these
programs are, as expected, already rejected when constructing the
variable-offset pointers. Note that the check in
check_stack_access_for_ptr_arithmetic() also prints "off=%d" while the
code removed by this patch does not (the error removed does not appear
in the "verification_error" values). For reproducibility, the repository
linked includes the raw data and scripts used to create the plot.

[1] https://arxiv.org/pdf/1807.03757.pdf
[2] https://gitlab.cs.fau.de/un65esoq/bpf-spectre/-/raw/53dc19fcf459c186613b1156a81504b39c8d49db/data/plots/23-02-26_23-56_bpftool/bpftool/0004-errors.pdf?inline=false

Fixes: 01f810ace9ed ("bpf: Allow variable-offset stack access")
Signed-off-by: Luis Gerhorst <gerhorst@cs.fau.de>
---
 kernel/bpf/verifier.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c13dd0f73e5..c1dc7fed655c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3958,16 +3958,13 @@ static int check_stack_read(struct bpf_verifier_env *env,
 	}
 	/* Variable offset is prohibited for unprivileged mode for simplicity
 	 * since it requires corresponding support in Spectre masking for stack
-	 * ALU. See also retrieve_ptr_limit().
+	 * ALU. See also retrieve_ptr_limit(). The check in
+	 * check_stack_access_for_ptr_arithmetic() called by
+	 * adjust_ptr_min_max_vals() prevents users from creating stack pointers
+	 * with variable offsets, therefore no check is required here. Further,
+	 * just checking it here would be insufficient as speculative stack
+	 * writes could still lead to unsafe speculative behaviour.
 	 */
-	if (!env->bypass_spec_v1 && var_off) {
-		char tn_buf[48];
-
-		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
-		verbose(env, "R%d variable offset stack access prohibited for !root, var_off=%s\n",
-				ptr_regno, tn_buf);
-		return -EACCES;
-	}
 
 	if (!var_off) {
 		off += reg->var_off.value;
-- 
2.34.1

Re: [PATCH] bpf: remove misleading spec_v1 check on var-offset stack read

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 15 Mar 2023 17:54:00 +0100 you wrote:
> For every BPF_ADD/SUB involving a pointer, adjust_ptr_min_max_vals()
> ensures that the resulting pointer has a constant offset if
> bypass_spec_v1 is false. This is ensured by calling
> sanitize_check_bounds() which in turn calls
> check_stack_access_for_ptr_arithmetic(). There, -EACCESS is returned if
> the register's offset is not constant, thereby rejecting the program.
> 
> [...]

Here is the summary with links:
  - bpf: remove misleading spec_v1 check on var-offset stack read
    https://git.kernel.org/bpf/bpf-next/c/082cdc69a465

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html