diff mbox series

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

Message ID 20230421162718.440230-6-daan.j.demeyer@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Add cgroup sockaddr hooks for unix sockets | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 43 this patch: 44
netdev/cc_maintainers warning 16 maintainers not CCed: davem@davemloft.net andrii@kernel.org pabeni@redhat.com song@kernel.org sdf@google.com haoluo@google.com yhs@fb.com kuba@kernel.org edumazet@google.com daniel@iogearbox.net john.fastabend@gmail.com kpsingh@kernel.org jolsa@kernel.org hawk@kernel.org netdev@vger.kernel.org ast@kernel.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 43 this patch: 44
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 fail Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 fail Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-33 fail Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 fail Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 fail Logs for test_verifier on x86_64 with llvm-16

Commit Message

Daan De Meyer April 21, 2023, 4:27 p.m. UTC 
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@gmail.com>
---
 net/core/filter.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov April 21, 2023, 9:01 p.m. UTC | #1 
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@gmail.com>
> ---
>  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.
Daan De Meyer April 24, 2023, 2:07 p.m. UTC | #2 
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@gmail.com>
> > ---
> >  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@gmail.com> 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@gmail.com>
> > ---
> >  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.
Alexei Starovoitov April 26, 2023, 12:10 a.m. UTC | #3 
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@gmail.com>
> > > ---
> > >  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 *.
Daan De Meyer April 26, 2023, 1:51 p.m. UTC | #4 
> 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@gmail.com>
> > > > ---
> > > >  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@gmail.com> 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@gmail.com>
> > > > ---
> > > >  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 *.
Martin KaFai Lau April 26, 2023, 9:30 p.m. UTC | #5 
On 4/21/23 9:27 AM, 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@gmail.com>
> ---
>   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)
> +{
> +	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);

Can you check whether the current inet* hooks can support changing sin[6]_* 
other than the sin[6]_addr part? e.g. from looking at inet6_getname(), bpf prog 
changing sin6_scope_id does not get through. Also, if I read the patches 
correctly, an increased sa_kern->uaddrlen is getting silently ignored for 
inet[6] hooks and it does not look right. e.g. for IPv6, what if the bpf prog 
set uaddrlen larger to include the sin6_scope_id?

or tweak the kfunc a little to only allow changing the sin[6]_addr and sun_path. 
Something like this (uncompiled code):

__bpf_kfunc int bpf_sock_addr_set(struct bpf_sock_addr_kern *sa_kern,
				  const u8 *addr, u32 addrlen__sz)
{
	struct sockaddr *sa = sa_kern->uaddr;
	struct sockaddr_in *sa4;
	struct sockaddr_in6 *sa6;
	struct sockaddr_un *un;

	switch (sa_kern->sk->sk_family) {
	case AF_INET:
		if (addrlen__sz != 4)
			return -EINVAL;
		sa4 = (struct sockaddr_in *)sa;
		sa4->sin_addr.s_addr = *(__be32 *)addr;
		break;
	case AF_INET6:
		if (addrlen__sz != 16)
			return -EINVAL;
		sa6 = (struct sockaddr_in6 *)sa;
		memcpy(sa6->sin6_addr.s6_addr, addr, 16);
		break;
	case AF_UNIX:
		if (addrlen__sz > UNIX_PATH_MAX)
			return -EINVAL;
		un = (struct sockaddr_un *)sa;
		memcpy(un->sun_path, addr, addrlen__sz);
		/* uaddrlen is only for the length of the sun_path
		 *(and sin[6]_addr too?)
		 */
		sa_kern->uaddrlen = addrlen__sz;
		break;
	default:
		/* impossible */
		WARN_ON_ONCE(1);
		return -EINVAL;
	}

	return 0;
}

> +	sa_kern->uaddrlen = addrlen__sz;
> +
> +	return 0;
> +}
>   __diag_pop();
diff mbox series

Patch

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)
+{
+	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)
+BTF_SET8_END(bpf_kfunc_check_set_sock_addr)
+
 static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
 	.owner = THIS_MODULE,
 	.set = &bpf_kfunc_check_set_skb,
@@ -11704,6 +11747,11 @@  static const struct btf_kfunc_id_set bpf_kfunc_set_xdp = {
 	.set = &bpf_kfunc_check_set_xdp,
 };
 
+static const struct btf_kfunc_id_set bpf_kfunc_set_sock_addr = {
+	.owner = THIS_MODULE,
+	.set = &bpf_kfunc_check_set_sock_addr,
+};
+
 static int __init bpf_kfunc_init(void)
 {
 	int ret;
@@ -11717,6 +11765,8 @@  static int __init bpf_kfunc_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_IN, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_XMIT, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
-	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
+	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
+						&bpf_kfunc_set_sock_addr);
 }
 late_initcall(bpf_kfunc_init);