Re: Packet pointers with 32-bit assignments

[Paul Chaignon <paul@isovalent.com>]

On Wed, Feb 02, 2022 at 07:24:20PM -0800, Yonghong Song wrote:
> 
> 
> On 2/2/22 12:59 PM, Paul Chaignon wrote:
> > Hi,
> > 
> > We're hitting the following verifier error in Cilium, on bpf-next
> > (86c7ecad3bf8) with LLVM 10.0.0 and mcpu=v3.
> > 
> >      ; return (void *)(unsigned long)ctx->data;
> >      2: (61) r9 = *(u32 *)(r7 +76)
> >      ; R7_w=ctx(id=0,off=0,imm=0) R9_w=pkt(id=0,off=0,r=0,imm=0)
> >      ; return (void *)(unsigned long)ctx->data;
> >      3: (bc) w6 = w9
> >      ; R6_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R9_w=pkt(id=0,off=0,r=0,imm=0)
> >      ; if (data + tot_len > data_end)
> >      4: (bf) r2 = r6
> >      ; R2_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv(id=1,umax_value=4294967295,var_off=(0x0; 0xffffffff))
> >      5: (07) r2 += 54
> >      ; R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff))
> >      ; if (data + tot_len > data_end)
> >      6: (2d) if r2 > r1 goto pc+466
> >      ; R1_w=pkt_end(id=0,off=0,imm=0) R2_w=inv(id=0,umin_value=54,umax_value=4294967349,var_off=(0x0; 0x1ffffffff))
> >      ; tmp = a->d1 - b->d1;
> >      7: (71) r2 = *(u8 *)(r6 +22)
> >      R6 invalid mem access 'inv'
> > 
> > As seen above, the verifier loses track of the packet pointer at
> > instruction 3, which then leads to an invalid memory access. Since
> > ctx->data is on 32 bits, LLVM generated a 32-bit assignment at
> > instruction 3.
> > 
> > We're usually able to avoid this by removing all 32-bit comparisons and
> > additions with the 64-bit variables for data and data_end. But in this
> > case, all variables are already on 64 bits.
> > 
> > Is there maybe a compiler patch we're missing which prevents such
> > assignments? If not, could we teach the verifier to track and convert
> > such assignments?
> 
> We kind of tackled this problem sometimes back. For example, the
> following is a proposed llvm builtin for this purpose:
>   https://reviews.llvm.org/D81479
>   https://reviews.llvm.org/D81480
> the builtin looks like
>   void *ptr = __builtin_bpf_load_u32_to_ptr(void *base,
>                   int const_offset);
> 
> The patches are abandoned since the functionality can be
> achieved with bpf asm code. Something likes below
>    asm("%0 = *(u32 *)(%1 + %2)" : "=r"(ptr) : "r"(ctx), "i"(76));
> We could define the above asm insn as a macro and put it
> in bpf_helpers.h.
> 
> Could you give a try?

It works fine! One more piece of asm bytecode in Cilium but who's
counting at this point :))

Thanks Yonghong!

> 
> > 
> > Regards,
> > Paul