On 06/19, Andrii Nakryiko wrote:
> On Wed, Jun 19, 2019 at 10:00 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> >
> > Implement new BPF_PROG_TYPE_CGROUP_SOCKOPT program type and
> > BPF_CGROUP_{G,S}ETSOCKOPT cgroup hooks.
> >
> > BPF_CGROUP_SETSOCKOPT get a read-only view of the setsockopt arguments.
> > BPF_CGROUP_GETSOCKOPT can modify the supplied buffer.
> > Both of them reuse existing PTR_TO_PACKET{,_END} infrastructure.
> >
> > The buffer memory is pre-allocated (because I don't think there is
> > a precedent for working with __user memory from bpf). This might be
> > slow to do for each {s,g}etsockopt call, that's why I've added
> > __cgroup_bpf_prog_array_is_empty that exits early if there is nothing
> > attached to a cgroup. Note, however, that there is a race between
> > __cgroup_bpf_prog_array_is_empty and BPF_PROG_RUN_ARRAY where cgroup
> > program layout might have changed; this should not be a problem
> > because in general there is a race between multiple calls to
> > {s,g}etsocktop and user adding/removing bpf progs from a cgroup.
> >
> > The return code of the BPF program is handled as follows:
> > * 0: EPERM
> > * 1: success, continue with next BPF program in the cgroup chain
> >
> > v7:
> > * return only 0 or 1 (Alexei Starovoitov)
> > * always run all progs (Alexei Starovoitov)
> > * use optval=0 as kernel bypass in setsockopt (Alexei Starovoitov)
> > (decided to use optval=-1 instead, optval=0 might be a valid input)
> > * call getsockopt hook after kernel handlers (Alexei Starovoitov)
> >
> > v6:
> > * rework cgroup chaining; stop as soon as bpf program returns
> > 0 or 2; see patch with the documentation for the details
> > * drop Andrii's and Martin's Acked-by (not sure they are comfortable
> > with the new state of things)
>
> I like the general approach, just overall unclear about seemingly
> artificial restrictions I mentioned below.
>
> >
> > v5:
> > * skip copy_to_user() and put_user() when ret == 0 (Martin Lau)
> >
> > v4:
> > * don't export bpf_sk_fullsock helper (Martin Lau)
> > * size != sizeof(__u64) for uapi pointers (Martin Lau)
> > * offsetof instead of bpf_ctx_range when checking ctx access (Martin Lau)
> >
> > v3:
> > * typos in BPF_PROG_CGROUP_SOCKOPT_RUN_ARRAY comments (Andrii Nakryiko)
> > * reverse christmas tree in BPF_PROG_CGROUP_SOCKOPT_RUN_ARRAY (Andrii
> > Nakryiko)
> > * use __bpf_md_ptr instead of __u32 for optval{,_end} (Martin Lau)
> > * use BPF_FIELD_SIZEOF() for consistency (Martin Lau)
> > * new CG_SOCKOPT_ACCESS macro to wrap repeated parts
> >
> > v2:
> > * moved bpf_sockopt_kern fields around to remove a hole (Martin Lau)
> > * aligned bpf_sockopt_kern->buf to 8 bytes (Martin Lau)
> > * bpf_prog_array_is_empty instead of bpf_prog_array_length (Martin Lau)
> > * added [0,2] return code check to verifier (Martin Lau)
> > * dropped unused buf[64] from the stack (Martin Lau)
> > * use PTR_TO_SOCKET for bpf_sockopt->sk (Martin Lau)
> > * dropped bpf_target_off from ctx rewrites (Martin Lau)
> > * use return code for kernel bypass (Martin Lau & Andrii Nakryiko)
> >
> > Cc: Martin Lau <kafai@xxxxxx>
> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > ---
>
> <snip>
>
> >
> > +struct bpf_sockopt_kern {
> > + struct sock *sk;
> > + u8 *optval;
> > + u8 *optval_end;
> > + s32 level;
> > + s32 optname;
> > + u32 optlen;
>
> Optlen is used below as signed integer, so switch it to s32?
Good catch, should be s32 here and below, thanks!
> > + s32 retval;
> > +
> > + /* Small on-stack optval buffer to avoid small allocations.
> > + */
> > + u8 buf[64] __aligned(8);
> > +};
> > +
>
> <snip>
>
> >
> > +struct bpf_sockopt {
> > + __bpf_md_ptr(struct bpf_sock *, sk);
> > + __bpf_md_ptr(void *, optval);
> > + __bpf_md_ptr(void *, optval_end);
> > +
> > + __s32 level;
> > + __s32 optname;
> > + __u32 optlen;
>
> Same as above, we expect BPF program to be able to set it to -1, so __s32?
>
> > + __s32 retval;
> > +};
> > +
> > #endif /* _UAPI__LINUX_BPF_H__ */
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>
> <snip>
>
> > +
> > + if (ctx.optlen == -1)
> > + /* optlen set to -1, bypass kernel */
> > + ret = 1;
> > + else if (ctx.optlen == optlen)
> > + /* optlen not changed, run kernel handler */
> > + ret = 0;
> > + else
> > + /* any other value is rejected */
> > + ret = -EFAULT;
>
> I'm consufed about this assymetry between getsockopt and setsockopt
> behavior. Why we are disallowing setsockopt from changing optlen (and
> value itself)? Is there any harm in allowing that? Imagining some use
> case that provides transparent "support" for some option, you'd need
> to be able to intercept and provide custom values both for setsockopt
> and getsockopt. So unless I'm missing some security implications, why
> not make both sides able to write?
Because kernel setsockopt handlers use get_user to read the data. We
can definitely allow changing optval+optlen, but we'd have to copy
that data back to userspace to let kernel handle it. I'm not sure how
userspace might feel about it. Can it be a buffer in the readonly
elf section?
> Similar will apply w.r.t. retval, why can't setsockopt return EINVAL
> to reject some options? This seems very useful and very similar to
> what sysctl BPF hooks do.
I was just being defensive because I'm not sure what's the use-case.
We can already return EPERM, why do we need to return a different
error code? Are we comfortable letting progs return arbitrary number?
Or you just want to allow a bunch of pre-defined error codes?
I haven't seen the ability to return arbitrary error from the sysctl
hooks, but maybe I didn't look hard enough.
> > +
> > +out:
> > + sockopt_free_buf(&ctx);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(__cgroup_bpf_run_filter_setsockopt);
> > +
> > +int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
> > + int optname, char __user *optval,
> > + int __user *optlen, int max_optlen,
> > + int retval)
> > +{
>
> <snip>
>
> > +
> > + if (ctx.optlen > max_optlen) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + /* BPF programs only allowed to set retval to 0, not some
> > + * arbitrary value.
> > + */
> > + if (ctx.retval != 0 && ctx.retval != retval) {
>
> Lookin at manpage of getsockopt, seems like at least two error codes
> are relevant and generally useful for BPF program to be able to
> return: EINVAL and ENOPROTOOPT? Why we are disallowing anything but 0
> (or preserving original retval)?
I was thinking about simple use-case where it's either BPF that
handles the opt or the kernel. And then it's BFP returning success or
EPERM. I don't think I understand why BPF needs to be able to
return different error codes. We can certainly do that if you think
that it makes sense; alternatively, we can start with 0 or kernel retval
and relax the requirements if someone really needs that in the future.
(I don't have a strong opinion here tbh).
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + if (copy_to_user(optval, ctx.optval, ctx.optlen) ||
> > + put_user(ctx.optlen, optlen)) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + ret = ctx.retval;
> > +
> > +out:
> > + sockopt_free_buf(&ctx);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(__cgroup_bpf_run_filter_getsockopt);
> > +
>
> <snip>
