netdev - Re: [PATCH bpf v2] bpf: increase {get,set}sockopt optval size limit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200605161314.GA245787@google.com>
Date:   Fri, 5 Jun 2020 09:13:14 -0700
From:   sdf@...gle.com
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        ast@...nel.org, daniel@...earbox.net
Subject: Re: [PATCH bpf v2] bpf: increase {get,set}sockopt optval size limit

On 06/04, Alexei Starovoitov wrote:
> On Thu, Jun 04, 2020 at 05:21:55PM -0700, Stanislav Fomichev wrote:
> > Attaching to these hooks can break iptables because its optval is
> > usually quite big, or at least bigger than the current PAGE_SIZE limit.
> >
> > There are two possible ways to fix it:
> > 1. Increase the limit to match iptables max optval.
> > 2. Implement some way to bypass the value if it's too big and trigger
> >    BPF only with level/optname so BPF can still decide whether
> >    to allow/deny big sockopts.
> >
> > I went with #1 which means we are potentially increasing the
> > amount of data we copy from the userspace from PAGE_SIZE to 512M.
> >
> > v2:
> > * proper comments formatting (Jakub Kicinski)
> >
> > Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  kernel/bpf/cgroup.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index fdf7836750a3..fb786b0f0f88 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -1276,7 +1276,14 @@ static bool  
> __cgroup_bpf_prog_array_is_empty(struct cgroup *cgrp,
> >
> >  static int sockopt_alloc_buf(struct bpf_sockopt_kern *ctx, int  
> max_optlen)
> >  {
> > -	if (unlikely(max_optlen > PAGE_SIZE) || max_optlen < 0)
> > +	/* The user with the largest known setsockopt optvals is iptables.
> > +	 * Allocate enough space to accommodate it.
> > +	 *
> > +	 * See XT_MAX_TABLE_SIZE and sizeof(struct ipt_replace).
> > +	 */
> > +	const int max_supported_optlen = 512 * 1024 * 1024 + 128;

> looks like arbitrary number. Why did you pick this one?
> Also it won't work with kzalloc() below.
I tried to add some reasoning that iptables is _probably_ the
biggest known user, but I agree, that is somewhat arbitrary.

And good point on kzalloc, iptables is using kvalloc, missed that :-(

> May be trim it to some number instead of hard failing ?
> bpf prog cannot really examine more than few kbytes.
I'm not sure we can trim, because if we do it and BPF program
modifies it, we need to merge the trimmed part with the
rest (untrimmed) before passing it down to the real kernel
handler. So it ether means we copy this modified part back
to the userspace (bad?) or we reallocate more memory (equally bad?).

Let me try to look into #2 that I've suggested in the description.
Maybe for "big" (>PAGE_SIZE) optvals we can say that only
level/optname are supported for some policy related actions,
but modifying/observing the data is not supported (at least for now).