diff mbox series

[bpf,5/9] bpf: restrict stack pointer arithmetic for unpriviledged

Message ID 20190101204925.4584-6-daniel@iogearbox.net
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series bpf fix to prevent oob under speculation | expand

Commit Message

Daniel Borkmann Jan. 1, 2019, 8:49 p.m. UTC 
Restrict stack pointer arithmetic for unpriviledged users in that
arithmetic itself must not go out of bounds as opposed to the actual
access later on. Therefore after each adjust_ptr_min_max_vals() with
a stack pointer as a destination we simulate a check_stack_access()
of 1 byte on the destination and once that fails the program is
rejected for unpriviledged program loads. This is analoge to map
value pointer arithmetic and needed for masking later on.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 63 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 22 deletions(-)

Comments

David Miller Jan. 1, 2019, 9:20 p.m. UTC | #1 
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue,  1 Jan 2019 21:49:21 +0100

> Restrict stack pointer arithmetic for unpriviledged users in that
> arithmetic itself must not go out of bounds as opposed to the actual
> access later on. Therefore after each adjust_ptr_min_max_vals() with
> a stack pointer as a destination we simulate a check_stack_access()
> of 1 byte on the destination and once that fails the program is
> rejected for unpriviledged program loads. This is analoge to map
                                                    ^^^^^^^

Should be "analog".
Daniel Borkmann Jan. 1, 2019, 10:57 p.m. UTC | #2 
On 01/01/2019 10:20 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Tue,  1 Jan 2019 21:49:21 +0100
> 
>> Restrict stack pointer arithmetic for unpriviledged users in that
>> arithmetic itself must not go out of bounds as opposed to the actual
>> access later on. Therefore after each adjust_ptr_min_max_vals() with
>> a stack pointer as a destination we simulate a check_stack_access()
>> of 1 byte on the destination and once that fails the program is
>> rejected for unpriviledged program loads. This is analoge to map
>                                                     ^^^^^^^
> 
> Should be "analog".

Thanks for spotting, I'll fix the typos up in v2 shortly.
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4da8c73..9ac205d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1387,6 +1387,31 @@  static int check_stack_read(struct bpf_verifier_env *env,
 	}
 }
 
+static int check_stack_access(struct bpf_verifier_env *env,
+			      const struct bpf_reg_state *reg,
+			      int off, int size)
+{
+	/* Stack accesses must be at a fixed offset, so that we
+	 * can determine what type of data were returned. See
+	 * check_stack_read().
+	 */
+	if (!tnum_is_const(reg->var_off)) {
+		char tn_buf[48];
+
+		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
+		verbose(env, "variable stack access var_off=%s off=%d size=%d",
+			tn_buf, off, size);
+		return -EACCES;
+	}
+
+	if (off >= 0 || off < -MAX_BPF_STACK) {
+		verbose(env, "invalid stack off=%d size=%d\n", off, size);
+		return -EACCES;
+	}
+
+	return 0;
+}
+
 /* check read/write into map element returned by bpf_map_lookup_elem() */
 static int __check_map_access(struct bpf_verifier_env *env, u32 regno, int off,
 			      int size, bool zero_size_allowed)
@@ -1954,24 +1979,10 @@  static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		}
 
 	} else if (reg->type == PTR_TO_STACK) {
-		/* stack accesses must be at a fixed offset, so that we can
-		 * determine what type of data were returned.
-		 * See check_stack_read().
-		 */
-		if (!tnum_is_const(reg->var_off)) {
-			char tn_buf[48];
-
-			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
-			verbose(env, "variable stack access var_off=%s off=%d size=%d",
-				tn_buf, off, size);
-			return -EACCES;
-		}
 		off += reg->var_off.value;
-		if (off >= 0 || off < -MAX_BPF_STACK) {
-			verbose(env, "invalid stack off=%d size=%d\n", off,
-				size);
-			return -EACCES;
-		}
+		err = check_stack_access(env, reg, off, size);
+		if (err)
+			return err;
 
 		state = func(env, reg);
 		err = update_stack_depth(env, state, off);
@@ -3253,11 +3264,19 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	/* For unprivileged we require that resulting offset must be in bounds
 	 * in order to be able to sanitize access later on.
 	 */
-	if (!env->allow_ptr_leaks && dst_reg->type == PTR_TO_MAP_VALUE &&
-	    check_map_access(env, dst, dst_reg->off, 1, false)) {
-		verbose(env, "R%d pointer arithmetic of map value goes out of range, prohibited for !root\n",
-			dst);
-		return -EACCES;
+	if (!env->allow_ptr_leaks) {
+		if (dst_reg->type == PTR_TO_MAP_VALUE &&
+		    check_map_access(env, dst, dst_reg->off, 1, false)) {
+			verbose(env, "R%d pointer arithmetic of map value goes out of range, "
+				"prohibited for !root\n", dst);
+			return -EACCES;
+		} else if (dst_reg->type == PTR_TO_STACK &&
+			   check_stack_access(env, dst_reg, dst_reg->off +
+					      dst_reg->var_off.value, 1)) {
+			verbose(env, "R%d stack pointer arithmetic goes out of range, "
+				"prohibited for !root\n", dst);
+			return -EACCES;
+		}
 	}
 
 	return 0;