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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200605043517.cupb77gzytqhanyk@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 4 Jun 2020 21:35:17 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.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 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.
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
>