On Mon, Apr 24, 2023 at 04:07:13PM +0200, Daan De Meyer wrote:
> bpf_sock_addr_set
> > On Fri, Apr 21, 2023 at 06:27:13PM +0200, Daan De Meyer wrote:
> > > As prep for adding unix socket support to the cgroup sockaddr hooks,
> > > let's add a kfunc bpf_sock_addr_set() that allows modifying the
> > > sockaddr length from bpf. This is required to allow modifying AF_UNIX
> > > sockaddrs correctly.
> > >
> > > Signed-off-by: Daan De Meyer <daan.j.demeyer@xxxxxxxxx>
> > > ---
> > > net/core/filter.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 51 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 44fb997434ad..1c656e2d7b58 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -81,6 +81,7 @@
> > > #include <net/xdp.h>
> > > #include <net/mptcp.h>
> > > #include <net/netfilter/nf_conntrack_bpf.h>
> > > +#include <linux/un.h>
> > >
> > > static const struct bpf_func_proto *
> > > bpf_sk_base_func_proto(enum bpf_func_id func_id);
> > > @@ -11670,6 +11671,44 @@ __bpf_kfunc int bpf_dynptr_from_xdp(struct xdp_buff *xdp, u64 flags,
> > >
> > > return 0;
> > > }
> > > +
> > > +__bpf_kfunc int bpf_sock_addr_set(struct bpf_sock_addr_kern *sa_kern,
> > > + const void *addr, u32 addrlen__sz)
> >
> > I think the verifier doesn't check validity of void* pointer for kfuncs.
> > Should it be 'struct sockaddr_un *' ?
>
> I had to make it 'void *' because the __sz tag only works on void
> pointers.
Why do you think so?
__bpf_kfunc struct nf_conn___init *
bpf_xdp_ct_alloc(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
enum hid_report_type rtype, enum hid_class_request reqtype)
> If it should be 'struct sockaddr_un *', how do I make the
> addrlen__sz tag work properly?
>
> > > +{
> > > + const struct sockaddr *sa = addr;
> > > +
> > > + if (addrlen__sz <= offsetof(struct sockaddr, sa_family))
> > > + return -EINVAL;
> > > +
> > > + if (addrlen__sz > sizeof(struct sockaddr_storage))
> > > + return -EINVAL;
> > > +
> > > + if (sa->sa_family != sa_kern->uaddr->sa_family)
> > > + return -EINVAL;
> > > +
> > > + switch (sa->sa_family) {
> > > + case AF_INET:
> > > + if (addrlen__sz < sizeof(struct sockaddr_in))
> > > + return -EINVAL;
> > > + break;
> > > + case AF_INET6:
> > > + if (addrlen__sz < SIN6_LEN_RFC2133)
> > > + return -EINVAL;
> > > + break;
> > > + case AF_UNIX:
> > > + if (addrlen__sz <= offsetof(struct sockaddr_un, sun_path) ||
> > > + addrlen__sz > sizeof(struct sockaddr_un))
> > > + return -EINVAL;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + memcpy(sa_kern->uaddr, sa, addrlen__sz);
> > > + sa_kern->uaddrlen = addrlen__sz;
> > > +
> > > + return 0;
> > > +}
> > > __diag_pop();
> > >
> > > int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
> > > @@ -11694,6 +11733,10 @@ BTF_SET8_START(bpf_kfunc_check_set_xdp)
> > > BTF_ID_FLAGS(func, bpf_dynptr_from_xdp)
> > > BTF_SET8_END(bpf_kfunc_check_set_xdp)
> > >
> > > +BTF_SET8_START(bpf_kfunc_check_set_sock_addr)
> > > +BTF_ID_FLAGS(func, bpf_sock_addr_set)
> >
> > It probably needs KF_TRUSTED_ARGS.
>
> If I understand the documentation of `KF_TRUSTED_ARGS` correctly, this
> wouldn't allow me to pass arbitrary sockaddr structs into
> bpf_sock_addr_set() anymore since those wouldn't be trusted?
KF_TRUSTED_ARGS won't allow legacy and untrusted PTR_TO_BTF_ID to
be passed into as 1st arg == struct bpf_sock_addr_kern *.
