The return for ctx_load_bytes() isn't being checked

[trvll]

In the datapath, the function ipv6_dec_hoplimit() makes a call to ctx_load_bytes() without checking for return errors. This will let this function code make a evaluation of an uninitialized variable (__u8 hl) when ctx_load_bytes() returned a error. In this case the eBPF compiler will generate a code that won't be accepted by Linux kernel verifier, throwing a "invalid size of register spill" error in the /var/log/syslog.

cilium/bpf/lib/ipv6.h

Line 151 in e9e4c46

ctx_load_bytes(ctx, off + offsetof(struct ipv6hdr, hop_limit),

Please, add error checks when calling ctx_load_bytes().

[trvll]

Hello @pchaigno, Could you assign it to me, pls?

[pchaigno]

In this case the eBPF compiler will generate a code that won't be accepted by Linux kernel verifier, throwing a "invalid size of register spill" error in the /var/log/syslog.

Could you describe exactly how you reproduced that error?

[trvll]

The problem occurs when the function skb_load_bytes() is introduced:

• Build Cilium with skb_load_bytes() using direct packet access (Changing skb_load_bytes to implement direct packet access. #15860)
• Install and start built agent on a NETNEXT node (make reload)
• Search /var/log/syslog for "invalid size of register spill"

[pchaigno]

Ok, I see 👍

AFAIK invalid size of register spill can't be reproduced from C code unless the compiler is buggy, hence my question. If we're using inline assembly, then that's totally possible.

[trvll]

YES, compiler should complain about possible usage of an uninitialized variable but does't while kernel verifier will prohibit loading the code for some reason not so clear for me, but when we do initialize the var accordingly or check the return error for ctx_load_bytes() in that context, the verifier will permit loading bpf_overlay.o.

[pchaigno]

No, I mean the compiler would not generated bytecode that results in invalid size of register spill. Uninitialized variable might happen but not invalid size of register spill.

compiler should complain about possible usage of an uninitialized variable

I think it is initialized by the helper from compiler's point of view.

Anyway, the C code is clearly incorrect and we should fix it 🙂