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. 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?
On Fri, 21 Apr 2023 at 23:01, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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 *' ?
>
> > +{
> > + 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.
