[bpf,v2] bpf: increase {get,set}sockopt optval size limit

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@google.com>
---
 kernel/bpf/cgroup.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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@google.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.
May be trim it to some number instead of hard failing ?
bpf prog cannot really examine more than few kbytes.

> +
> +	if (unlikely(max_optlen > max_supported_optlen) || max_optlen < 0)
>  		return -EINVAL;
>  
>  	ctx->optval = kzalloc(max_optlen, GFP_USER);
> -- 
> 2.27.0.278.ge193c7cf3a9-goog
>

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@google.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).

From: Stanislav Fomichev
> Sent: 05 June 2020 01:22
> 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.
...
> +	const int max_supported_optlen = 512 * 1024 * 1024 + 128;

512MB seems a bit big.
I'd have thought that iptables would be usable from a 32bit application
where that is 1/6th the process address space.
Anything that might be that big ought to be done in chunks.

I was looking at the SCTP socket option code.
ISTR that may require just over 256kB - still silly, but not as bad.

SCTP also requires that getsockopt() copy the buffer in from userspace.
One call required more than a 'sockaddr storage' be read in.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)