Re: BPF CO-RE and array fields in context struct

[Yonghong Song <yhs@fb.com>]

On 11/22/21 8:19 AM, YiFei Zhu wrote:
> Hi
> 
> I've been investigating the use of BPF CO-RE. I discovered that if I
> include vmlinux.h and have all structures annotated with
> __attribute__((preserve_access_index)), including the context struct,
> then a prog that accesses an array field in the context struct, in
> some particular way, cannot pass the verifier.
> 
> A bunch of manual reduction plus creduce gives me this output:
> 
>    struct bpf_sock_ops {
>      int family;
>      int remote_ip6[];
>    } __attribute__((preserve_access_index));
>    __attribute__((section("sockops"))) int b(struct bpf_sock_ops *d) {
>      int a = d->family;
>      int *c = d->remote_ip6;
>      c[2] = a;
>      return 0;
>    }
> 
> With Debian clang version 11.1.0-4+build1, this compiles to
> 
>    0000000000000000 <b>:
>           0: b7 02 00 00 04 00 00 00 r2 = 4
>           1: bf 13 00 00 00 00 00 00 r3 = r1
>           2: 0f 23 00 00 00 00 00 00 r3 += r2
>           3: 61 11 00 00 00 00 00 00 r1 = *(u32 *)(r1 + 0)
>           4: 63 13 08 00 00 00 00 00 *(u32 *)(r3 + 8) = r1
>           5: b7 00 00 00 00 00 00 00 r0 = 0
>           6: 95 00 00 00 00 00 00 00 exit
> 
> And the prog will be rejected with this verifier log:
> 
>    ; __attribute__((section("sockops"))) int b(struct bpf_sock_ops *d) {
>    0: (b7) r2 = 32
>    1: (bf) r3 = r1
>    2: (0f) r3 += r2
>    last_idx 2 first_idx 0
>    regs=4 stack=0 before 1: (bf) r3 = r1
>    regs=4 stack=0 before 0: (b7) r2 = 32
>    ; int a = d->family;
>    3: (61) r1 = *(u32 *)(r1 +20)
>    ; c[2] = a;
>    4: (63) *(u32 *)(r3 +8) = r1
>    dereference of modified ctx ptr R3 off=32 disallowed
>    processed 5 insns (limit 1000000) max_states_per_insn 0 total_states
> 0 peak_states 0 mark_read 0

Thanks for reporting the issue. The example you had here exposed an llvm 
limitation.

For the following code:
   >      int *c = d->remote_ip6;
   >      c[2] = a;

The relocation will apply to d->remote_ip6. And the below code sequence
is for c = d->remote_ip6:

 >    0: (b7) r2 = 32
 >    1: (bf) r3 = r1
 >    2: (0f) r3 += r2

And later c[2] store has the issue as you described above.
Note that llvm does not generate relocation for array access itself.
It needs to be part of access chain like d->remote_ip6[2] to be
relocatable.

> 
> Looking at check_ctx_reg() and its callsite at check_mem_access() in
> verifier.c, it seems that the verifier really does not like when the
> context pointer has an offset, in this case the field offset of
> d->remote_ip6.
> 
> I thought this is just an issue with array fields, that field offset
> relocations may have trouble expressing two field accesses (one struct
> member, one array memory). However, further testing reveals that this
> is not the case, because if I simplify out the local variables, the
> error is gone:
> 
>    struct bpf_sock_ops {
>      int family;
>      int remote_ip6[];
>    } __attribute__((preserve_access_index));
>    __attribute__((section("sockops"))) int b(struct bpf_sock_ops *d) {
>      d->remote_ip6[2] = d->family;
>      return 0;
>    }
> 
> is compiled to:
> 
>    0000000000000000 <b>:
>           0: 61 12 00 00 00 00 00 00 r2 = *(u32 *)(r1 + 0)
>           1: 63 21 0c 00 00 00 00 00 *(u32 *)(r1 + 12) = r2
>           2: b7 00 00 00 00 00 00 00 r0 = 0
>           3: 95 00 00 00 00 00 00 00 exit
> 
> and is loaded as:
> 
>    ; d->remote_ip6[2] = d->family;
>    0: (61) r2 = *(u32 *)(r1 +20)
>    ; d->remote_ip6[2] = d->family;
>    1: (63) *(u32 *)(r1 +40) = r2
>    invalid bpf_context access off=40 size=4
> 
> I believe this error is because d->remote_ip6 is read-only, that this
> modification might be more of a product of creduce, but we can see
> that the CO-RE adjected offset of the array element from the context
> pointer is correct: 32 to remote_ip6, 8 array index, so total offset
> is 40.

In this case, the statement is
    d->remote_ip6[2] = d->family;

And the whole "d->remote_ip6[2]" is relocated. So we generate a single 
instruction for it:
    *(u32 *)(r1 +40) = ...

So the workaround is to have all related field in the statement up to
the load/store operation so we have ONE complete relocation.

> 
> Also note that removal of __attribute__((preserve_access_index)) from
> the first (miscompiled) program produces exactly the same bytecode as
> this new program (with no locals).
> 
> What is going on here? Why does the access of an array in context in
> this particular way cause it to generate code that would not pass the
> verifier? Is it a bug in Clang/LLVM, or is it the verifier being too
> strict?

How can we fix this issue? We could generate IR with relocation 
information for standalone array operation and later llvm can chain
them together. I will take a further look later for a fix.

> 
> Thanks
> YiFei Zhu
>