bpf: verifier: Refactor helper access type tracking

Previously, the verifier was treating all PTR_TO_STACK registers passed
to a helper call as potentially written to by the helper. However, all
calls to check_stack_range_initialized() already have precise access type
information available.

Rather than treat ACCESS_HELPER as a proxy for BPF_WRITE, pass
enum bpf_access_type to check_stack_range_initialized() to more
precisely track helper arguments.

One benefit from this precision is that registers tracked as valid
spills and passed as a read-only helper argument remain tracked after
the call.  Rather than being marked STACK_MISC afterwards.

An additional benefit is the verifier logs are also more precise. For
this particular error, users will enjoy a slightly clearer message. See
included selftest updates for examples.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
Link: https://lore.kernel.org/r/ff885c0e5859e0cd12077c3148ff0754cad4f7ed.1736886479.git.dxu@dxuuu.xyz
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8879977eb9eba6197c77b1f5f6a289ffbef03f84..b71858390e650bf794e467b2db402b480220b35d 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5303,7 +5303,7 @@ enum bpf_access_src {
 static int check_stack_range_initialized(struct bpf_verifier_env *env,
                                         int regno, int off, int access_size,
                                         bool zero_size_allowed,
-                                        enum bpf_access_src type,
+                                        enum bpf_access_type type,
                                         struct bpf_call_arg_meta *meta);
 
 static struct bpf_reg_state *reg_state(struct bpf_verifier_env *env, int regno)
@@ -5336,7 +5336,7 @@ static int check_stack_read_var_off(struct bpf_verifier_env *env,
        /* Note that we pass a NULL meta, so raw access will not be permitted.
         */
        err = check_stack_range_initialized(env, ptr_regno, off, size,
-                                           false, ACCESS_DIRECT, NULL);
+                                           false, BPF_READ, NULL);
        if (err)
                return err;
 
@@ -7190,7 +7190,7 @@ static int check_stack_slot_within_bounds(struct bpf_verifier_env *env,
 static int check_stack_access_within_bounds(
                struct bpf_verifier_env *env,
                int regno, int off, int access_size,
-               enum bpf_access_src src, enum bpf_access_type type)
+               enum bpf_access_type type)
 {
        struct bpf_reg_state *regs = cur_regs(env);
        struct bpf_reg_state *reg = regs + regno;
@@ -7199,10 +7199,7 @@ static int check_stack_access_within_bounds(
        int err;
        char *err_extra;
 
-       if (src == ACCESS_HELPER)
-               /* We don't know if helpers are reading or writing (or both). */
-               err_extra = " indirect access to";
-       else if (type == BPF_READ)
+       if (type == BPF_READ)
                err_extra = " read from";
        else
                err_extra = " write to";
@@ -7420,7 +7417,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 
        } else if (reg->type == PTR_TO_STACK) {
                /* Basic bounds checks. */
-               err = check_stack_access_within_bounds(env, regno, off, size, ACCESS_DIRECT, t);
+               err = check_stack_access_within_bounds(env, regno, off, size, t);
                if (err)
                        return err;
 
@@ -7640,13 +7637,11 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 static int check_stack_range_initialized(
                struct bpf_verifier_env *env, int regno, int off,
                int access_size, bool zero_size_allowed,
-               enum bpf_access_src type, struct bpf_call_arg_meta *meta)
+               enum bpf_access_type type, struct bpf_call_arg_meta *meta)
 {
        struct bpf_reg_state *reg = reg_state(env, regno);
        struct bpf_func_state *state = func(env, reg);
        int err, min_off, max_off, i, j, slot, spi;
-       char *err_extra = type == ACCESS_HELPER ? " indirect" : "";
-       enum bpf_access_type bounds_check_type;
        /* Some accesses can write anything into the stack, others are
         * read-only.
         */
@@ -7657,18 +7652,10 @@ static int check_stack_range_initialized(
                return -EACCES;
        }
 
-       if (type == ACCESS_HELPER) {
-               /* The bounds checks for writes are more permissive than for
-                * reads. However, if raw_mode is not set, we'll do extra
-                * checks below.
-                */
-               bounds_check_type = BPF_WRITE;
+       if (type == BPF_WRITE)
                clobber = true;
-       } else {
-               bounds_check_type = BPF_READ;
-       }
-       err = check_stack_access_within_bounds(env, regno, off, access_size,
-                                              type, bounds_check_type);
+
+       err = check_stack_access_within_bounds(env, regno, off, access_size, type);
        if (err)
                return err;
 
@@ -7685,8 +7672,8 @@ static int check_stack_range_initialized(
                        char tn_buf[48];
 
                        tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
-                       verbose(env, "R%d%s variable offset stack access prohibited for !root, var_off=%s\n",
-                               regno, err_extra, tn_buf);
+                       verbose(env, "R%d 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
@@ -7767,14 +7754,14 @@ static int check_stack_range_initialized(
                }
 
                if (tnum_is_const(reg->var_off)) {
-                       verbose(env, "invalid%s read from stack R%d off %d+%d size %d\n",
-                               err_extra, regno, min_off, i - min_off, access_size);
+                       verbose(env, "invalid read from stack R%d off %d+%d size %d\n",
+                               regno, min_off, i - min_off, access_size);
                } else {
                        char tn_buf[48];
 
                        tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
-                       verbose(env, "invalid%s read from stack R%d var_off %s+%d size %d\n",
-                               err_extra, regno, tn_buf, i - min_off, access_size);
+                       verbose(env, "invalid read from stack R%d var_off %s+%d size %d\n",
+                               regno, tn_buf, i - min_off, access_size);
                }
                return -EACCES;
 mark:
@@ -7849,7 +7836,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
                return check_stack_range_initialized(
                                env,
                                regno, reg->off, access_size,
-                               zero_size_allowed, ACCESS_HELPER, meta);
+                               zero_size_allowed, access_type, meta);
        case PTR_TO_BTF_ID:
                return check_ptr_to_btf_access(env, regs, regno, reg->off,
                                               access_size, BPF_READ, -1);

diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index dfd817d0348c473dc47cdb63dada22e609cc8eca..bd8f15229f5c736abe34187af2e6b450d6fa4b7c 100644 (file)
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -192,7 +192,7 @@ done:
 
 /* Can't add a dynptr to a map */
 SEC("?raw_tp")
-__failure __msg("invalid indirect read from stack")
+__failure __msg("invalid read from stack")
 int add_dynptr_to_map1(void *ctx)
 {
        struct bpf_dynptr ptr;
@@ -210,7 +210,7 @@ int add_dynptr_to_map1(void *ctx)
 
 /* Can't add a struct with an embedded dynptr to a map */
 SEC("?raw_tp")
-__failure __msg("invalid indirect read from stack")
+__failure __msg("invalid read from stack")
 int add_dynptr_to_map2(void *ctx)
 {
        struct test_info x;
@@ -398,7 +398,7 @@ int data_slice_missing_null_check2(void *ctx)
  * dynptr argument
  */
 SEC("?raw_tp")
-__failure __msg("invalid indirect read from stack")
+__failure __msg("invalid read from stack")
 int invalid_helper1(void *ctx)
 {
        struct bpf_dynptr ptr;

diff --git a/tools/testing/selftests/bpf/progs/test_global_func10.c b/tools/testing/selftests/bpf/progs/test_global_func10.c
index 5da001ca57a5f1da67dd74dc26c157f479e30a7d..09d027bd3ea80973b2f1a3cf0345b660a86014bb 100644 (file)
--- a/tools/testing/selftests/bpf/progs/test_global_func10.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func10.c
@@ -26,7 +26,7 @@ __noinline int foo(const struct Big *big)
 }
 
 SEC("cgroup_skb/ingress")
-__failure __msg("invalid indirect access to stack")
+__failure __msg("invalid read from stack")
 int global_func10(struct __sk_buff *skb)
 {
        const struct Small small = {.x = skb->len };

diff --git a/tools/testing/selftests/bpf/progs/uninit_stack.c b/tools/testing/selftests/bpf/progs/uninit_stack.c
index 8a403470e557f879067940cd632d9ee2357b5c02..046a204c8fc697bb04de5c79309f3c19713f6922 100644 (file)
--- a/tools/testing/selftests/bpf/progs/uninit_stack.c
+++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
@@ -70,7 +70,8 @@ __naked int helper_uninit_to_misc(void *ctx)
                r1 = r10;                               \
                r1 += -128;                             \
                r2 = 32;                                \
-               call %[bpf_trace_printk];               \
+               r3 = 0;                                 \
+               call %[bpf_probe_read_user];            \
                /* Call to dummy() forces print_verifier_state(..., true),      \
                 * thus showing the stack state, matched by __msg().            \
                 */                                     \
@@ -79,7 +80,7 @@ __naked int helper_uninit_to_misc(void *ctx)
                exit;                                   \
 "
                      :
-                     : __imm(bpf_trace_printk),
+                     : __imm(bpf_probe_read_user),
                        __imm(dummy)
                      : __clobber_all);
 }

diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
index 8d77cc5323d3371b191937dd844aaa8bb2deb6f3..fb62e09f21146d10c384687949dc1028ac803d33 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
@@ -28,7 +28,7 @@ __naked void stack_out_of_bounds(void)
 SEC("socket")
 __description("uninitialized stack1")
 __success __log_level(4) __msg("stack depth 8")
-__failure_unpriv __msg_unpriv("invalid indirect read from stack")
+__failure_unpriv __msg_unpriv("invalid read from stack")
 __naked void uninitialized_stack1(void)
 {
        asm volatile ("                                 \

diff --git a/tools/testing/selftests/bpf/progs/verifier_const_or.c b/tools/testing/selftests/bpf/progs/verifier_const_or.c
index ba8922b2eebd9aace069edc45a46926f009a1a02..68c568c3c3a019b4103c0d07c87d09aca26935ea 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_const_or.c
+++ b/tools/testing/selftests/bpf/progs/verifier_const_or.c
@@ -25,7 +25,7 @@ __naked void constant_should_keep_constant_type(void)
 
 SEC("tracepoint")
 __description("constant register |= constant should not bypass stack boundary checks")
-__failure __msg("invalid indirect access to stack R1 off=-48 size=58")
+__failure __msg("invalid write to stack R1 off=-48 size=58")
 __naked void not_bypass_stack_boundary_checks_1(void)
 {
        asm volatile ("                                 \
@@ -62,7 +62,7 @@ __naked void register_should_keep_constant_type(void)
 
 SEC("tracepoint")
 __description("constant register |= constant register should not bypass stack boundary checks")
-__failure __msg("invalid indirect access to stack R1 off=-48 size=58")
+__failure __msg("invalid write to stack R1 off=-48 size=58")
 __naked void not_bypass_stack_boundary_checks_2(void)
 {
        asm volatile ("                                 \

diff --git a/tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c b/tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c
index 50c6b22606f6952660b09264aa67702da23ff961..f2c54e4d89ebef04e411fa2a2c512c901901328a 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c
+++ b/tools/testing/selftests/bpf/progs/verifier_helper_access_var_len.c
@@ -67,7 +67,7 @@ SEC("socket")
 __description("helper access to variable memory: stack, bitwise AND, zero included")
 /* in privileged mode reads from uninitialized stack locations are permitted */
 __success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R2 off -64+0 size 64")
+__msg_unpriv("invalid read from stack R2 off -64+0 size 64")
 __retval(0)
 __naked void stack_bitwise_and_zero_included(void)
 {
@@ -100,7 +100,7 @@ __naked void stack_bitwise_and_zero_included(void)
 
 SEC("tracepoint")
 __description("helper access to variable memory: stack, bitwise AND + JMP, wrong max")
-__failure __msg("invalid indirect access to stack R1 off=-64 size=65")
+__failure __msg("invalid write to stack R1 off=-64 size=65")
 __naked void bitwise_and_jmp_wrong_max(void)
 {
        asm volatile ("                                 \
@@ -187,7 +187,7 @@ l0_%=:      r0 = 0;                                         \
 
 SEC("tracepoint")
 __description("helper access to variable memory: stack, JMP, bounds + offset")
-__failure __msg("invalid indirect access to stack R1 off=-64 size=65")
+__failure __msg("invalid write to stack R1 off=-64 size=65")
 __naked void memory_stack_jmp_bounds_offset(void)
 {
        asm volatile ("                                 \
@@ -211,7 +211,7 @@ l0_%=:      r0 = 0;                                         \
 
 SEC("tracepoint")
 __description("helper access to variable memory: stack, JMP, wrong max")
-__failure __msg("invalid indirect access to stack R1 off=-64 size=65")
+__failure __msg("invalid write to stack R1 off=-64 size=65")
 __naked void memory_stack_jmp_wrong_max(void)
 {
        asm volatile ("                                 \
@@ -260,7 +260,7 @@ SEC("socket")
 __description("helper access to variable memory: stack, JMP, no min check")
 /* in privileged mode reads from uninitialized stack locations are permitted */
 __success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R2 off -64+0 size 64")
+__msg_unpriv("invalid read from stack R2 off -64+0 size 64")
 __retval(0)
 __naked void stack_jmp_no_min_check(void)
 {
@@ -750,7 +750,7 @@ SEC("socket")
 __description("helper access to variable memory: 8 bytes leak")
 /* in privileged mode reads from uninitialized stack locations are permitted */
 __success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R2 off -64+32 size 64")
+__msg_unpriv("invalid read from stack R2 off -64+32 size 64")
 __retval(0)
 __naked void variable_memory_8_bytes_leak(void)
 {

diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
index 5f2efb895edbb235a4491d46dc6a78dfca4e87fd..59e34d5586543e22de9d2d284ea0efb18d342dee 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
@@ -96,7 +96,7 @@ __naked void arg_ptr_to_long_misaligned(void)
 
 SEC("cgroup/sysctl")
 __description("arg pointer to long size < sizeof(long)")
-__failure __msg("invalid indirect access to stack R4 off=-4 size=8")
+__failure __msg("invalid write to stack R4 off=-4 size=8")
 __naked void to_long_size_sizeof_long(void)
 {
        asm volatile ("                                 \

diff --git a/tools/testing/selftests/bpf/progs/verifier_mtu.c b/tools/testing/selftests/bpf/progs/verifier_mtu.c
index 4ccf1ebc42d168cd45dcb136135ffd9e01565de1..256956ea1ac51bbff853ac30a97287b39ecd2528 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
+++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
@@ -8,7 +8,7 @@ SEC("tc/ingress")
 __description("uninit/mtu: write rejected")
 __success
 __caps_unpriv(CAP_BPF|CAP_NET_ADMIN)
-__failure_unpriv __msg_unpriv("invalid indirect read from stack")
+__failure_unpriv __msg_unpriv("invalid read from stack")
 int tc_uninit_mtu(struct __sk_buff *ctx)
 {
        __u32 mtu;

diff --git a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
index 7cc83acac7271d60f91b0fbc94ec919cf2f8ddab..c689665e07b976b24a2bc8966d00b34689fdf1e9 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
@@ -236,7 +236,7 @@ __naked void load_bytes_spilled_regs_data(void)
 
 SEC("tc")
 __description("raw_stack: skb_load_bytes, invalid access 1")
-__failure __msg("invalid indirect access to stack R3 off=-513 size=8")
+__failure __msg("invalid write to stack R3 off=-513 size=8")
 __naked void load_bytes_invalid_access_1(void)
 {
        asm volatile ("                                 \
@@ -255,7 +255,7 @@ __naked void load_bytes_invalid_access_1(void)
 
 SEC("tc")
 __description("raw_stack: skb_load_bytes, invalid access 2")
-__failure __msg("invalid indirect access to stack R3 off=-1 size=8")
+__failure __msg("invalid write to stack R3 off=-1 size=8")
 __naked void load_bytes_invalid_access_2(void)
 {
        asm volatile ("                                 \

diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index 7ea535bfbacd3e8c01a61442a46be1107fec04fd..a4a5e2071604047cd4711da0497f1fd1772f9c4f 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -199,7 +199,7 @@ __naked void pass_pointer_to_helper_function(void)
 SEC("socket")
 __description("unpriv: indirectly pass pointer on stack to helper function")
 __success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R2 off -8+0 size 8")
+__msg_unpriv("invalid read from stack R2 off -8+0 size 8")
 __retval(0)
 __naked void on_stack_to_helper_function(void)
 {

diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
index c810f4f6f479c6dcc5c4448168fe9b2233a64d18..1d36d01b746e78d1895db3a6f96a99314051d1a3 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
+++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
@@ -203,7 +203,7 @@ __naked void stack_write_clobbers_spilled_regs(void)
 
 SEC("sockops")
 __description("indirect variable-offset stack access, unbounded")
-__failure __msg("invalid unbounded variable-offset indirect access to stack R4")
+__failure __msg("invalid unbounded variable-offset write to stack R4")
 __naked void variable_offset_stack_access_unbounded(void)
 {
        asm volatile ("                                 \
@@ -236,7 +236,7 @@ l0_%=:      r0 = 0;                                         \
 
 SEC("lwt_in")
 __description("indirect variable-offset stack access, max out of bound")
-__failure __msg("invalid variable-offset indirect access to stack R2")
+__failure __msg("invalid variable-offset read from stack R2")
 __naked void access_max_out_of_bound(void)
 {
        asm volatile ("                                 \
@@ -269,7 +269,7 @@ __naked void access_max_out_of_bound(void)
  */
 SEC("socket")
 __description("indirect variable-offset stack access, zero-sized, max out of bound")
-__failure __msg("invalid variable-offset indirect access to stack R1")
+__failure __msg("invalid variable-offset write to stack R1")
 __naked void zero_sized_access_max_out_of_bound(void)
 {
        asm volatile ("                      \
@@ -294,7 +294,7 @@ __naked void zero_sized_access_max_out_of_bound(void)
 
 SEC("lwt_in")
 __description("indirect variable-offset stack access, min out of bound")
-__failure __msg("invalid variable-offset indirect access to stack R2")
+__failure __msg("invalid variable-offset read from stack R2")
 __naked void access_min_out_of_bound(void)
 {
        asm volatile ("                                 \

diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index 7afc2619ab146fd98cb36d61ceb7cf2cb656c614..18596ae0b0c19fdbb6705a807bc919b9fb352d4a 100644 (file)
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -2252,7 +2252,7 @@
        BPF_EXIT_INSN(),
        },
        .fixup_map_hash_48b = { 7 },
-       .errstr_unpriv = "invalid indirect read from stack R2 off -8+0 size 8",
+       .errstr_unpriv = "invalid read from stack R2 off -8+0 size 8",
        .result_unpriv = REJECT,
        /* in privileged mode reads from uninitialized stack locations are permitted */
        .result = ACCEPT,