bpf: split check_mem_access logic for map values

Move the logic to check memory accesses to a PTR_TO_MAP_VALUE_ADJ from
check_mem_access() to a separate helper check_map_access_adj(). This
enables to use those checks in other parts of the verifier as well,
where boundaries on PTR_TO_MAP_VALUE_ADJ might need to be checked, for
example when checking helper function arguments. The same thing is
already happening for other types such as PTR_TO_PACKET and its
check_packet_access() helper.

The code has been copied verbatim, with the only difference of removing
the "off += reg->max_value" statement and moving the sum into the call
statement to check_map_access(), as that was only needed due to the
earlier common check_map_access() call.

Signed-off-by: Gianluca Borello <g.borello@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 83ed2f8..8333fbc 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -635,6 +635,51 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno, int off,
        return 0;
 }
 
+/* check read/write into an adjusted map element */
+static int check_map_access_adj(struct bpf_verifier_env *env, u32 regno,
+                               int off, int size)
+{
+       struct bpf_verifier_state *state = &env->cur_state;
+       struct bpf_reg_state *reg = &state->regs[regno];
+       int err;
+
+       /* We adjusted the register to this map value, so we
+        * need to change off and size to min_value and max_value
+        * respectively to make sure our theoretical access will be
+        * safe.
+        */
+       if (log_level)
+               print_verifier_state(state);
+       env->varlen_map_value_access = true;
+       /* The minimum value is only important with signed
+        * comparisons where we can't assume the floor of a
+        * value is 0.  If we are using signed variables for our
+        * index'es we need to make sure that whatever we use
+        * will have a set floor within our range.
+        */
+       if (reg->min_value < 0) {
+               verbose("R%d min value is negative, either use unsigned index or do a if (index >=0) check.\n",
+                       regno);
+               return -EACCES;
+       }
+       err = check_map_access(env, regno, reg->min_value + off, size);
+       if (err) {
+               verbose("R%d min value is outside of the array range\n",
+                       regno);
+               return err;
+       }
+
+       /* If we haven't set a max value then we need to bail
+        * since we can't be sure we won't do bad things.
+        */
+       if (reg->max_value == BPF_REGISTER_MAX_RANGE) {
+               verbose("R%d unbounded memory access, make sure to bounds check any array access into a map\n",
+                       regno);
+               return -EACCES;
+       }
+       return check_map_access(env, regno, reg->max_value + off, size);
+}
+
 #define MAX_PACKET_OFF 0xffff
 
 static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
@@ -775,45 +820,10 @@ static int check_mem_access(struct bpf_verifier_env *env, u32 regno, int off,
                        return -EACCES;
                }
 
-               /* If we adjusted the register to this map value at all then we
-                * need to change off and size to min_value and max_value
-                * respectively to make sure our theoretical access will be
-                * safe.
-                */
-               if (reg->type == PTR_TO_MAP_VALUE_ADJ) {
-                       if (log_level)
-                               print_verifier_state(state);
-                       env->varlen_map_value_access = true;
-                       /* The minimum value is only important with signed
-                        * comparisons where we can't assume the floor of a
-                        * value is 0.  If we are using signed variables for our
-                        * index'es we need to make sure that whatever we use
-                        * will have a set floor within our range.
-                        */
-                       if (reg->min_value < 0) {
-                               verbose("R%d min value is negative, either use unsigned index or do a if (index >=0) check.\n",
-                                       regno);
-                               return -EACCES;
-                       }
-                       err = check_map_access(env, regno, reg->min_value + off,
-                                              size);
-                       if (err) {
-                               verbose("R%d min value is outside of the array range\n",
-                                       regno);
-                               return err;
-                       }
-
-                       /* If we haven't set a max value then we need to bail
-                        * since we can't be sure we won't do bad things.
-                        */
-                       if (reg->max_value == BPF_REGISTER_MAX_RANGE) {
-                               verbose("R%d unbounded memory access, make sure to bounds check any array access into a map\n",
-                                       regno);
-                               return -EACCES;
-                       }
-                       off += reg->max_value;
-               }
-               err = check_map_access(env, regno, off, size);
+               if (reg->type == PTR_TO_MAP_VALUE_ADJ)
+                       err = check_map_access_adj(env, regno, off, size);
+               else
+                       err = check_map_access(env, regno, off, size);
                if (!err && t == BPF_READ && value_regno >= 0)
                        mark_reg_unknown_value(state->regs, value_regno);