Merge branch 'bpf-varstack-fixes'

Andrey Ignatov says:

====================
v2->v3:
- sanity check max value for variable offset.

v1->v2:
- rely on meta = NULL to reject var_off stack access to uninit buffer.

This patch set is a follow-up for discussion [1].

It fixes variable offset stack access handling for raw and unprivileged
mode, rejecting both of them, and sanity checks max variable offset value.

Patch 1 handles raw (uninitialized) mode.
Patch 2 adds test for raw mode.
Patch 3 handles unprivileged mode.
Patch 4 adds test for unprivileged mode.
Patch 5 adds sanity check for max value of variable offset.
Patch 6 adds test for variable offset max value checking.
Patch 7 is a minor fix in verbose log.

Unprivileged mode is an interesting case since one (and only?) way to come
up with variable offset is to use pointer arithmetics. Though pointer
arithmetics is already prohibited for unprivileged mode. I'm not sure if
it's enough though and it seems like a good idea to still reject variable
offset for unpriv in check_stack_boundary(). Please see patches 3 and 4
for more details on this.

[1] https://marc.info/?l=linux-netdev&m=155419526427742&w=2
====================

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

2 files changed, 150 insertions, 6 deletions

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bb27b675923c..48718e1da16d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1426,7 +1426,7 @@ static int check_stack_access(struct bpf_verifier_env *env,
                 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",
+                verbose(env, "variable stack access var_off=%s off=%d size=%d\n",
                         tn_buf, off, size);
                 return -EACCES;
         }
@@ -2226,16 +2226,50 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
                 if (err)
                         return err;
         } else {
+                /* 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().
+                 */
+                if (!env->allow_ptr_leaks) {
+                        char tn_buf[48];
+
+                        tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
+                        verbose(env, "R%d indirect variable offset stack access prohibited for !root, var_off=%s\n",
+                                regno, tn_buf);
+                        return -EACCES;
+                }
+                /* Only initialized buffer on stack is allowed to be accessed
+                 * with variable offset. With uninitialized buffer it's hard to
+                 * guarantee that whole memory is marked as initialized on
+                 * helper return since specific bounds are unknown what may
+                 * cause uninitialized stack leaking.
+                 */
+                if (meta && meta->raw_mode)
+                        meta = NULL;
+
+                if (reg->smax_value >= BPF_MAX_VAR_OFF ||
+                    reg->smax_value <= -BPF_MAX_VAR_OFF) {
+                        verbose(env, "R%d unbounded indirect variable offset stack access\n",
+                                regno);
+                        return -EACCES;
+                }
                 min_off = reg->smin_value + reg->off;
-                max_off = reg->umax_value + reg->off;
+                max_off = reg->smax_value + reg->off;
                 err = __check_stack_boundary(env, regno, min_off, access_size,
                                              zero_size_allowed);
-                if (err)
+                if (err) {
+                        verbose(env, "R%d min value is outside of stack bound\n",
+                                regno);
                         return err;
+                }
                 err = __check_stack_boundary(env, regno, max_off, access_size,
                                              zero_size_allowed);
-                if (err)
+                if (err) {
+                        verbose(env, "R%d max value is outside of stack bound\n",
+                                regno);
                         return err;
+                }
         }
 
         if (meta && meta->raw_mode) {
@@ -3330,6 +3364,9 @@ static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg,
 
         switch (ptr_reg->type) {
         case PTR_TO_STACK:
+                /* Indirect variable offset stack access is prohibited in
+                 * unprivileged mode so it's not handled here.
+                 */
                 off = ptr_reg->off + ptr_reg->var_off.value;
                 if (mask_to_left)
                         *ptr_limit = MAX_BPF_STACK + off;
diff --git a/tools/testing/selftests/bpf/verifier/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
index c4ebd0bb0781..8504ac937809 100644
--- a/tools/testing/selftests/bpf/verifier/var_off.c
+++ b/tools/testing/selftests/bpf/verifier/var_off.c
@@ -40,7 +40,35 @@
         .prog_type = BPF_PROG_TYPE_LWT_IN,
 },
 {
-        "indirect variable-offset stack access, out of bound",
+        "indirect variable-offset stack access, unbounded",
+        .insns = {
+        BPF_MOV64_IMM(BPF_REG_2, 6),
+        BPF_MOV64_IMM(BPF_REG_3, 28),
+        /* Fill the top 16 bytes of the stack. */
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0),
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+        /* Get an unknown value. */
+        BPF_LDX_MEM(BPF_DW, BPF_REG_4, BPF_REG_1, offsetof(struct bpf_sock_ops,
+                                                           bytes_received)),
+        /* Check the lower bound but don't check the upper one. */
+        BPF_JMP_IMM(BPF_JSLT, BPF_REG_4, 0, 4),
+        /* Point the lower bound to initialized stack. Offset is now in range
+         * from fp-16 to fp+0x7fffffffffffffef, i.e. max value is unbounded.
+         */
+        BPF_ALU64_IMM(BPF_SUB, BPF_REG_4, 16),
+        BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_10),
+        BPF_MOV64_IMM(BPF_REG_5, 8),
+        /* Dereference it indirectly. */
+        BPF_EMIT_CALL(BPF_FUNC_getsockopt),
+        BPF_MOV64_IMM(BPF_REG_0, 0),
+        BPF_EXIT_INSN(),
+        },
+        .errstr = "R4 unbounded indirect variable offset stack access",
+        .result = REJECT,
+        .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+},
+{
+        "indirect variable-offset stack access, max out of bound",
         .insns = {
         /* Fill the top 8 bytes of the stack */
         BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
@@ -60,7 +88,32 @@
         BPF_EXIT_INSN(),
         },
         .fixup_map_hash_8b = { 5 },
-        .errstr = "invalid stack type R2 var_off",
+        .errstr = "R2 max value is outside of stack bound",
+        .result = REJECT,
+        .prog_type = BPF_PROG_TYPE_LWT_IN,
+},
+{
+        "indirect variable-offset stack access, min out of bound",
+        .insns = {
+        /* Fill the top 8 bytes of the stack */
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+        /* Get an unknown value */
+        BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, 0),
+        /* Make it small and 4-byte aligned */
+        BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 4),
+        BPF_ALU64_IMM(BPF_SUB, BPF_REG_2, 516),
+        /* add it to fp.  We now have either fp-516 or fp-512, but
+         * we don't know which
+         */
+        BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_10),
+        /* dereference it indirectly */
+        BPF_LD_MAP_FD(BPF_REG_1, 0),
+        BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+        BPF_MOV64_IMM(BPF_REG_0, 0),
+        BPF_EXIT_INSN(),
+        },
+        .fixup_map_hash_8b = { 5 },
+        .errstr = "R2 min value is outside of stack bound",
         .result = REJECT,
         .prog_type = BPF_PROG_TYPE_LWT_IN,
 },
@@ -115,6 +168,60 @@
         .prog_type = BPF_PROG_TYPE_LWT_IN,
 },
 {
+        "indirect variable-offset stack access, priv vs unpriv",
+        .insns = {
+        /* Fill the top 16 bytes of the stack. */
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0),
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+        /* Get an unknown value. */
+        BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, 0),
+        /* Make it small and 4-byte aligned. */
+        BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 4),
+        BPF_ALU64_IMM(BPF_SUB, BPF_REG_2, 16),
+        /* Add it to fp.  We now have either fp-12 or fp-16, we don't know
+         * which, but either way it points to initialized stack.
+         */
+        BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_10),
+        /* Dereference it indirectly. */
+        BPF_LD_MAP_FD(BPF_REG_1, 0),
+        BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
+        BPF_MOV64_IMM(BPF_REG_0, 0),
+        BPF_EXIT_INSN(),
+        },
+        .fixup_map_hash_8b = { 6 },
+        .errstr_unpriv = "R2 stack pointer arithmetic goes out of range, prohibited for !root",
+        .result_unpriv = REJECT,
+        .result = ACCEPT,
+        .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+},
+{
+        "indirect variable-offset stack access, uninitialized",
+        .insns = {
+        BPF_MOV64_IMM(BPF_REG_2, 6),
+        BPF_MOV64_IMM(BPF_REG_3, 28),
+        /* Fill the top 16 bytes of the stack. */
+        BPF_ST_MEM(BPF_W, BPF_REG_10, -16, 0),
+        BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+        /* Get an unknown value. */
+        BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_1, 0),
+        /* Make it small and 4-byte aligned. */
+        BPF_ALU64_IMM(BPF_AND, BPF_REG_4, 4),
+        BPF_ALU64_IMM(BPF_SUB, BPF_REG_4, 16),
+        /* Add it to fp.  We now have either fp-12 or fp-16, we don't know
+         * which, but either way it points to initialized stack.
+         */
+        BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_10),
+        BPF_MOV64_IMM(BPF_REG_5, 8),
+        /* Dereference it indirectly. */
+        BPF_EMIT_CALL(BPF_FUNC_getsockopt),
+        BPF_MOV64_IMM(BPF_REG_0, 0),
+        BPF_EXIT_INSN(),
+        },
+        .errstr = "invalid indirect read from stack var_off",
+        .result = REJECT,
+        .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+},
+{
         "indirect variable-offset stack access, ok",
         .insns = {
         /* Fill the top 16 bytes of the stack. */