> 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)
> 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 *.
Because of the following check in get_kfunc_ptr_arg_type():
if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(env,
meta->btf, ref_t, 0) &&
(arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
verbose(env, "arg#%d pointer type %s %s must point to
%sscalar, or struct with scalar\n",
argno, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
return -EINVAL;
}
I was hitting this error when addr was sockaddr_un * instead of void
*. Will KF_TRUSTED_ARGS
bypass that check?
On Wed, 26 Apr 2023 at 02:10, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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 *.
