Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Mon, 16 Oct 2023 14:50:05 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Sergei Trofimovich <slyich@gmail.com>
Cc:     linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Eric Biederman <ebiederm@xmission.com>, linux-mm@kvack.org
Subject: Re: [RESEND PATCH] uapi: increase MAX_ARG_STRLEN from 128K to 6M
Message-ID: <202310161439.BEA904AEE8@keescook>
References: <20231016212507.131902-1-slyich@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231016212507.131902-1-slyich@gmail.com>
Precedence: bulk

On Mon, Oct 16, 2023 at 10:25:07PM +0100, Sergei Trofimovich wrote:
> Before the change linux allowed individual execve() arguments or
> environment variable entries to be only as big as 32 pages.
> 
> Histroically before b6a2fea3931 "mm: variable length argument support"
> MAX_ARG_STRLEN used to be full allowed size `argv[] + envp[]`.
> 
> When full limit was abandoned individual parameters were still limited
> by a safe limit of 128K.
> 
> Nowadays' linux allows `argv[]+envp[]` to be as laerge as 6MB (3/4
> `_STK_LIM`).

See bprm_stack_limits() for the details, but yes, 3/4 _STK_LIM tends to
be the max, unless the rlim_stack is set smaller.

> Some build systems like `autoconf` use a single environment variable
> to pass `CFLAGS` environment variable around. It's not a bug problem
> if the argument list is short.
> 
> But some packaging systems prefer installing each package into
> individual directory. As a result that requires quite long string of
> parameters like:
> 
>     CFLAGS="-I/path/to/pkg1 -I/path/to/pkg2 ..."
> 
> This can easily overflow 128K and does happen for `NixOS` and `nixpkgs`
> repositories on a regular basis.

That's ... alarming. What are you doing currently to work around this?

> 
> Similar pattern is exhibited by `gcc` which converts it's input command
> line into a single environment variable (https://gcc.gnu.org/PR111527):
> 
>   $ big_100k_var=$(printf "%0*d" 100000 0)
> 
>   # this works: 200KB of options for `printf` external command
>   $ $(which printf) "%s %s" $big_100k_var $big_100k_var >/dev/null; echo $?
>   0
> 
>   # this fails: 200KB of options for `gcc`, fails in `cc1`
>   $ touch a.c; gcc -c a.c -DA=$big_100k_var -DB=$big_100k_var
>   gcc: fatal error: cannot execute 'cc1': execv: Argument list too long
>   compilation terminated.
> 
> I would say this 128KB limitation is arbitrary.
> The change raises the limit of `MAX_ARG_STRLEN` from 32 pakes (128K
> n `x86_64`) to the maximum limit of stack allowed by Linux today.
> 
> It has a minor chance of overflowing userspace programs that use
> `MAX_ARG_STRLEN` to allocate the strings on stack. It should not be a
> big problem as such programs are already are at risk of overflowing
> stack.
> 
> Tested as:
>     $ V=$(printf "%*d" 1000000 0) ls
> 
> Before the change it failed with `ls: Argument list too long`. After the
> change `ls` executes as expected.
> 
> WDYT of abandoning the limit and allow user to fill entire environment
> with a single command or a single variable?

Changing this value shouldn't risk any vma collisions, since exec is
still checking the utilization before starting the actual process
replacement steps (see bprm->argmin).

It does seem pathological to set this to the full 6MB, though, since
that would _immediately_ get rejected by execve() with an -E2BIG, but
ultimately, there isn't really any specific limit to the length of
individual strings as long as the entire space is less than
bprm->argmin.

Perhaps MAX_ARG_STRLEN should be removed entirely and the kernel can
just use bprm->argmin? I haven't really looked at that to see if it's
sane, though. It just feels like MAX_ARG_STRLEN isn't a meaningful
limit.

-Kees

> 
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Eric Biederman <ebiederm@xmission.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: linux-mm@kvack.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Sergei Trofimovich <slyich@gmail.com>
> ---
>  include/uapi/linux/binfmts.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/binfmts.h b/include/uapi/linux/binfmts.h
> index c6f9450efc12..4e828515a22e 100644
> --- a/include/uapi/linux/binfmts.h
> +++ b/include/uapi/linux/binfmts.h
> @@ -8,11 +8,11 @@ struct pt_regs;
>  
>  /*
>   * These are the maximum length and maximum number of strings passed to the
> - * execve() system call.  MAX_ARG_STRLEN is essentially random but serves to
> - * prevent the kernel from being unduly impacted by misaddressed pointers.
> + * execve() system call.  MAX_ARG_STRLEN is as large as Linux allows new
> + * stack to grow.  Currently it's `_STK_LIM / 4 * 3 = 6MB` (see fs/exec.c).
>   * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
>   */
> -#define MAX_ARG_STRLEN (PAGE_SIZE * 32)
> +#define MAX_ARG_STRLEN (6 * 1024 * 1024)
>  #define MAX_ARG_STRINGS 0x7FFFFFFF
>  
>  /* sizeof(linux_binprm->buf) */
> -- 
> 2.42.0
> 

-- 
Kees Cook