On Mon, May 6, 2024 at 2:39 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 4/30/24 5:18 AM, Philo Lu wrote:
> > Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for skb
> > parsing, especially for non-linear paged skb data. This is achieved by
> > adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
> > for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
> > excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
> >
> > We also need the skb dynptr to be read-only in tp_btf. Because
> > may_access_direct_pkt_data() returns false by default when checking
> > bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING to it
> > explicitly.
> >
> > Signed-off-by: Philo Lu <lulie@xxxxxxxxxxxxxxxxx>
> > ---
> > net/core/filter.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 786d792ac816..399492970b8c 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
> > }
> >
> > BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
> > -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
> > +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
>
> I can see the usefulness of having the same way parsing the header as the
> tc-bpf. However, it implicitly means the skb->data and skb_shinfo are trusted
> also. afaik, it should be as long as skb is not NULL.
>
> From looking at include/trace/events, there is case that skb is NULL. e.g.
> tcp_send_reset. It is not something new though, e.g. using skb->sk in the tp_btf
> could be bad already. This should be addressed before allowing more kfunc/helper.
Good catch.
We need to fix this part first:
if (prog_args_trusted(prog))
info->reg_type |= PTR_TRUSTED;
Brute force fix by adding PTR_MAYBE_NULL is probably overkill.
I suspect passing NULL into tracepoint is more of an exception than the rule.
Maybe we can use kfunc's "__" suffix approach for tracepoint args?
[43947] FUNC_PROTO '(anon)' ret_type_id=0 vlen=4
'__data' type_id=10
'sk' type_id=3434
'skb' type_id=2386
'reason' type_id=39860
[43948] FUNC '__bpf_trace_tcp_send_reset' type_id=43947 linkage=static
Then do:
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 49b5ee091cf6..325e8a31729a 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -91,7 +91,7 @@ DEFINE_RST_REASON(FN, FN)
TRACE_EVENT(tcp_send_reset,
TP_PROTO(const struct sock *sk,
- const struct sk_buff *skb,
+ const struct sk_buff *skb__nullable,
and detect it in btf_ctx_access().
