Re: [PATCH bpf-next v3 05/10] bpf: Add bpf_sock_addr_set() to allow writing sockaddr len from bpf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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 *.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux