On Fri, May 5, 2023 at 11:49 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 5/4/23 5:13 PM, Martin KaFai Lau wrote:
> >
> > Follow up on the v6 patch-set regarding KF_TRUSTED_ARGS.
> > KF_TRUSTED_ARGS is needed here to avoid the cases where a PTR_TO_BTF_ID sk is
> > obtained by following another pointer. eg. getting a sk pointer (may be even
> > NULL) by following another sk pointer. The recent PTR_TRUSTED concept in the
> > verifier can guard this. I tried and the following should do:
> >
> > diff --git i/net/core/filter.c w/net/core/filter.c
> > index 68b228f3eca6..d82e038da0e3 100644
> > --- i/net/core/filter.c
> > +++ w/net/core/filter.c
> > @@ -11767,7 +11767,7 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
> > __diag_pop()
> >
> > BTF_SET8_START(sock_destroy_kfunc_set)
> > -BTF_ID_FLAGS(func, bpf_sock_destroy)
> > +BTF_ID_FLAGS(func, bpf_sock_destroy, KF_TRUSTED_ARGS)
> > BTF_SET8_END(sock_destroy_kfunc_set)
> >
> > static int tracing_iter_filter(const struct bpf_prog *prog, u32 kfunc_id)
> > diff --git i/net/ipv4/tcp_ipv4.c w/net/ipv4/tcp_ipv4.c
> > index 887f83a90d85..a769284e8291 100644
> > --- i/net/ipv4/tcp_ipv4.c
> > +++ w/net/ipv4/tcp_ipv4.c
> > @@ -3354,7 +3354,7 @@ static struct bpf_iter_reg tcp_reg_info = {
> > .ctx_arg_info_size = 1,
> > .ctx_arg_info = {
> > { offsetof(struct bpf_iter__tcp, sk_common),
> > - PTR_TO_BTF_ID_OR_NULL },
> > + PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
>
> Alexei, what do you think about having "PTR_MAYBE_NULL | PTR_TRUSTED" here?
> The verifier side looks fine (eg. is_trusted_reg() is taking PTR_MAYBE_NULL into
> consideration). However, it seems this will be the first "PTR_MAYBE_NULL |
> PTR_TRUSTED" use case and not sure if PTR_MAYBE_NULL may conceptually conflict
> with the PTR_TRUSTED idea (like PTR_TRUSTED should not be NULL).
Conceptually it should be fine. There are no real cases of
PTR_TRUSTED | PTR_MAYBE_NULL now, though check_reg_type() handles it.
Proceed with care, I guess :)
