diff mbox series

[bpf-next,v3,1/2] bpf: permit map_ptr arithmetic with opcode add and offset 0

Message ID 20200908175702.2463625-1-yhs@fb.com
State New
Headers show
Series bpf: permit map_ptr arithmetic with opcode add and offset 0 | expand

Commit Message

Yonghong Song Sept. 8, 2020, 5:57 p.m. UTC
Commit 41c48f3a98231 ("bpf: Support access
to bpf map fields") added support to access map fields
with CORE support. For example,

            struct bpf_map {
                    __u32 max_entries;
            } __attribute__((preserve_access_index));

            struct bpf_array {
                    struct bpf_map map;
                    __u32 elem_size;
            } __attribute__((preserve_access_index));

            struct {
                    __uint(type, BPF_MAP_TYPE_ARRAY);
                    __uint(max_entries, 4);
                    __type(key, __u32);
                    __type(value, __u32);
            } m_array SEC(".maps");

            SEC("cgroup_skb/egress")
            int cg_skb(void *ctx)
            {
                    struct bpf_array *array = (struct bpf_array *)&m_array;

                    /* .. array->map.max_entries .. */
            }

In kernel, bpf_htab has similar structure,

	    struct bpf_htab {
		    struct bpf_map map;
                    ...
            }

In the above cg_skb(), to access array->map.max_entries, with CORE, the clang will
generate two builtin's.
            base = &m_array;
            /* access array.map */
            map_addr = __builtin_preserve_struct_access_info(base, 0, 0);
            /* access array.map.max_entries */
            max_entries_addr = __builtin_preserve_struct_access_info(map_addr, 0, 0);
	    max_entries = *max_entries_addr;

In the current llvm, if two builtin's are in the same function or
in the same function after inlining, the compiler is smart enough to chain
them together and generates like below:
            base = &m_array;
            max_entries = *(base + reloc_offset); /* reloc_offset = 0 in this case */
and we are fine.

But if we force no inlining for one of functions in test_map_ptr() selftest, e.g.,
check_default(), the above two __builtin_preserve_* will be in two different
functions. In this case, we will have code like:
   func check_hash():
            reloc_offset_map = 0;
            base = &m_array;
            map_base = base + reloc_offset_map;
            check_default(map_base, ...)
   func check_default(map_base, ...):
            max_entries = *(map_base + reloc_offset_max_entries);

In kernel, map_ptr (CONST_PTR_TO_MAP) does not allow any arithmetic.
The above "map_base = base + reloc_offset_map" will trigger a verifier failure.
  ; VERIFY(check_default(&hash->map, map));
  0: (18) r7 = 0xffffb4fe8018a004
  2: (b4) w1 = 110
  3: (63) *(u32 *)(r7 +0) = r1
   R1_w=invP110 R7_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R10=fp0
  ; VERIFY_TYPE(BPF_MAP_TYPE_HASH, check_hash);
  4: (18) r1 = 0xffffb4fe8018a000
  6: (b4) w2 = 1
  7: (63) *(u32 *)(r1 +0) = r2
   R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0) R2_w=invP1 R7_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R10=fp0
  8: (b7) r2 = 0
  9: (18) r8 = 0xffff90bcb500c000
  11: (18) r1 = 0xffff90bcb500c000
  13: (0f) r1 += r2
  R1 pointer arithmetic on map_ptr prohibited

To fix the issue, let us permit map_ptr + 0 arithmetic which will
result in exactly the same map_ptr.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/verifier.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andrii Nakryiko Sept. 8, 2020, 11:11 p.m. UTC | #1
On Tue, Sep 8, 2020 at 10:58 AM Yonghong Song <yhs@fb.com> wrote:
>
> Commit 41c48f3a98231 ("bpf: Support access
> to bpf map fields") added support to access map fields
> with CORE support. For example,
>
>             struct bpf_map {
>                     __u32 max_entries;
>             } __attribute__((preserve_access_index));
>
>             struct bpf_array {
>                     struct bpf_map map;
>                     __u32 elem_size;
>             } __attribute__((preserve_access_index));
>
>             struct {
>                     __uint(type, BPF_MAP_TYPE_ARRAY);
>                     __uint(max_entries, 4);
>                     __type(key, __u32);
>                     __type(value, __u32);
>             } m_array SEC(".maps");
>
>             SEC("cgroup_skb/egress")
>             int cg_skb(void *ctx)
>             {
>                     struct bpf_array *array = (struct bpf_array *)&m_array;
>
>                     /* .. array->map.max_entries .. */
>             }
>
> In kernel, bpf_htab has similar structure,
>
>             struct bpf_htab {
>                     struct bpf_map map;
>                     ...
>             }
>
> In the above cg_skb(), to access array->map.max_entries, with CORE, the clang will
> generate two builtin's.
>             base = &m_array;
>             /* access array.map */
>             map_addr = __builtin_preserve_struct_access_info(base, 0, 0);
>             /* access array.map.max_entries */
>             max_entries_addr = __builtin_preserve_struct_access_info(map_addr, 0, 0);
>             max_entries = *max_entries_addr;
>
> In the current llvm, if two builtin's are in the same function or
> in the same function after inlining, the compiler is smart enough to chain
> them together and generates like below:
>             base = &m_array;
>             max_entries = *(base + reloc_offset); /* reloc_offset = 0 in this case */
> and we are fine.
>
> But if we force no inlining for one of functions in test_map_ptr() selftest, e.g.,
> check_default(), the above two __builtin_preserve_* will be in two different
> functions. In this case, we will have code like:
>    func check_hash():
>             reloc_offset_map = 0;
>             base = &m_array;
>             map_base = base + reloc_offset_map;
>             check_default(map_base, ...)
>    func check_default(map_base, ...):
>             max_entries = *(map_base + reloc_offset_max_entries);
>
> In kernel, map_ptr (CONST_PTR_TO_MAP) does not allow any arithmetic.
> The above "map_base = base + reloc_offset_map" will trigger a verifier failure.
>   ; VERIFY(check_default(&hash->map, map));
>   0: (18) r7 = 0xffffb4fe8018a004
>   2: (b4) w1 = 110
>   3: (63) *(u32 *)(r7 +0) = r1
>    R1_w=invP110 R7_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R10=fp0
>   ; VERIFY_TYPE(BPF_MAP_TYPE_HASH, check_hash);
>   4: (18) r1 = 0xffffb4fe8018a000
>   6: (b4) w2 = 1
>   7: (63) *(u32 *)(r1 +0) = r2
>    R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0) R2_w=invP1 R7_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R10=fp0
>   8: (b7) r2 = 0
>   9: (18) r8 = 0xffff90bcb500c000
>   11: (18) r1 = 0xffff90bcb500c000
>   13: (0f) r1 += r2
>   R1 pointer arithmetic on map_ptr prohibited
>
> To fix the issue, let us permit map_ptr + 0 arithmetic which will
> result in exactly the same map_ptr.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

[...]
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b4e9c56b8b32..814bc6c1ad16 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5317,6 +5317,10 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 			dst, reg_type_str[ptr_reg->type]);
 		return -EACCES;
 	case CONST_PTR_TO_MAP:
+		/* smin_val represents the known value */
+		if (known && smin_val == 0 && opcode == BPF_ADD)
+			break;
+		/* fall-through */
 	case PTR_TO_PACKET_END:
 	case PTR_TO_SOCKET:
 	case PTR_TO_SOCKET_OR_NULL: