On Mon Jan 6, 2025 at 9:34 PM CET, Eduard Zingerman wrote:
> On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
[...]
> > +#define TEST(NAME, CALLEE) \
> > + SEC("socket") \
> > + __description("r0: " #NAME) \
> > + __failure __msg("math between ctx pointer and register with unbounded min value") \
> > + __naked int check_abnormal_ret_r0_##NAME(void) \
> > + { \
> > + asm volatile(" \
> > + r6 = r1; \
> > + r2 = r10; \
> > + r2 += -8; \
> > + call " #CALLEE "; \
> > + r6 += r0; \
> > + r0 = 0; \
> > + exit; \
> > + " : \
> > + : \
> > + : __clobber_all); \
> > + } \
> > + \
> > + SEC("socket") \
> > + __description("ref: " #NAME) \
> > + __failure __msg("math between ctx pointer and register with unbounded min value") \
> > + __naked int check_abnormal_ret_ref_##NAME(void) \
> > + { \
> > + asm volatile(" \
> > + r6 = r1; \
> > + r7 = r10; \
> > + r7 += -8; \
> > + r2 = r7; \
> > + call " #CALLEE "; \
> > + r0 = *(u64*)(r7 + 0); \
> > + r6 += r0; \
> > + exit; \
> > + " : \
> > + : \
> > + : __clobber_all); \
> > + }
>
> Nit: I think having both cases is an overkill, as both effectively
> test if branching occur.
Fair enough, I can drop the reference tests.
> [...]
>
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> > + __uint(max_entries, 1);
> > + __uint(key_size, sizeof(int));
> > + __array(values, void(void));
> > +} map_prog SEC(".maps") = {
> > + .values = {
> > + [0] = (void *)&dummy_prog,
> > + },
> > +};
> > +
> > +static __noinline __used
> > +int callee_tail_call(struct __sk_buff *skb, __u64 *foo)
> > +{
> > + bpf_tail_call(skb, &map_prog, 0);
> > + *foo = 1;
> > + return 0;
> > +}
>
> Nit: I'd also add a test where invalid action is taken
> after bpf_tail_call inside the callee,
> just to make sure that both branches are explored.
Good idea, I'll add that in and resend. Thanks for the feedback!
>
> > +
> > +SEC("socket")
> > +__description("r0 not set by tail_call")
> > +__failure __msg("R0 !read_ok")
> > +int check_abnormal_ret_tail_call_fail(struct __sk_buff *skb)
> > +{
> > + return bpf_tail_call(skb, &map_prog, 0);
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
