Re: [Qemu-devel] [PATCH] linux-user: limit number of arguments to execve

[Eric Blake]

On 03/03/2017 05:25 AM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> Limit the number of arguments passed to execve(2) call from
> a user program, as large number of them could lead to a bad
> guest address error.

Depending on how the kernel was compiled, you may have a limited maximum
size for the combined storage used by argc, environ, and all pointers.
Older kernels had a hard ceiling on storage used by argc and environ as
small as 128k (not just the bytes pointed to, but also the storage
occupied by the pointers) - it was possible to trigger E2BIG failures
with a large number of arguments in the array, but also with a single
argument that was too long.  Modern kernels have increased the limits
somewhat (now it is as large as 1/4 available stack size), but still has
maximum sizes for a single argument (larger than before, but still
finite).  Any one of these E2BIG failures is something we need to handle
gracefully, especially if the guest is expecting to be able to use a
larger limit than the host is supporting.  So I'm not sure that limiting
the number of arguments is sufficient - you may also have to limit the
size of the arguments.

I typed the above without looking at your patch.  Now that I've done
that, I see that you are tackling yet another type of corruption...

> 
> Reported-by: Jann Horn <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  linux-user/syscall.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9be8e95..c545c12 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7766,6 +7766,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #endif
>      case TARGET_NR_execve:
>          {
> +#define ARG_MAX 65535
>              char **argp, **envp;
>              int argc, envc;
>              abi_ulong gp;
> @@ -7794,6 +7795,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  envc++;
>              }
>  
> +            if (argc > ARG_MAX || envc > ARG_MAX) {
> +                fprintf(stderr,
> +                        "argc(%d), envc(%d) exceed %d\n", argc, envc, 
> ARG_MAX);

Isn't there something better than fprintf() for reporting errors?

> +                ret = -TARGET_EFAULT;
> +                break;
> +            }
>              argp = alloca((argc + 1) * sizeof(void *));
>              envp = alloca((envc + 1) * sizeof(void *));

...Uggh. You're using alloca() but allowing an allocation of way more
than 4k.  That means a guest can cause corruption of the stack (or, with
large enough arguments, even escape out of the stack) before you even
get to the execve() call to even worry about E2BIG issues.  Even though
your new limit of 64k argc/envc limits the damage compared to what it
used to be, you are still prone to an allocation that walks beyond the
guard page and causes bad behavior.  Either you need the limits to be
much smaller, or you should consider using the heap instead of the stack
(alloca should never be used for more than about 4k).  And there's still
the possibility that even with your cap, that you are not handling E2BIG
correctly.  You'll need to respin this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature