[Qemu-devel] [PATCH v2 0/2] Limit and protect execve arguments

[Qemu-devel] [PATCH v2 0/2] Limit and protect execve arguments

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

A user program could pass large number of 'argv','env' arguments to an
execve(2) call. It could lead to bad behaviour as the TARGET_NR_execve:
allocates stack memory(via alloca) for these arguments. alloca(3) is
better for allocations of upto one page(4KB) of stack memory. As anything
more could smash stack protectors in place.

This patch(v2) set attempts to fix these issues.
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html

Thank you.
--
Prasad J Pandit (2):
  linux-user: limit number of arguments to execve
  linux-user: allocate heap memory for execve arguments

 linux-user/syscall.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--
2.9.3

[Qemu-devel] [PATCH v2 1/2] linux-user: limit number of arguments to execve

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 linux-user/syscall.c | 6 ++++++
 1 file changed, 6 insertions(+)

Update per: use gemu_log() to report error
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9be8e95..86a4a9c 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,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 envc++;
             }
 
+            if (argc > ARG_MAX || envc > ARG_MAX) {
+                gemu_log("argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
+                ret = -TARGET_E2BIG;
+                break;
+            }
             argp = alloca((argc + 1) * sizeof(void *));
             envp = alloca((envc + 1) * sizeof(void *));
 
-- 
2.9.3

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

[Peter Maydell]

On 6 March 2017 at 07:17, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  linux-user/syscall.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> Update per: use gemu_log() to report error
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9be8e95..86a4a9c 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,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  envc++;
>              }
>
> +            if (argc > ARG_MAX || envc > ARG_MAX) {
> +                gemu_log("argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
> +                ret = -TARGET_E2BIG;
> +                break;
> +            }
>              argp = alloca((argc + 1) * sizeof(void *));
>              envp = alloca((envc + 1) * sizeof(void *));


We need to fix this by not using alloca(), not by imposing
an arbitrary limit that's still rather over-large for an
alloca allocation, as Eric suggested.

thanks
-- PMM

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On 03/06/2017 09:42 AM, Peter Maydell wrote:
> On 6 March 2017 at 07:17, P J P <ppandit@redhat.com> wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> 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.
>>
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  linux-user/syscall.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>

>>          {
>> +#define ARG_MAX 65535
>>              char **argp, **envp;
>>              int argc, envc;
>>              abi_ulong gp;
>> @@ -7794,6 +7795,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>                  envc++;
>>              }
>>
>> +            if (argc > ARG_MAX || envc > ARG_MAX) {
>> +                gemu_log("argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
>> +                ret = -TARGET_E2BIG;
>> +                break;
>> +            }
>>              argp = alloca((argc + 1) * sizeof(void *));
>>              envp = alloca((envc + 1) * sizeof(void *));
> 
> 
> We need to fix this by not using alloca(), not by imposing
> an arbitrary limit that's still rather over-large for an
> alloca allocation, as Eric suggested.

And patch 2/2 does that. Does that patch in isolation fix the problem?
In which case, we don't want this patch.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Arguments passed to execve(2) call from user program could
be large, allocating stack memory for them via alloca(3) call
would lead to bad behaviour. Use 'g_malloc0' to allocate memory
for such arguments.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 linux-user/syscall.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Update per: replace alloca() with g_malloc0()
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 86a4a9c..404fb0b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7800,8 +7800,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 ret = -TARGET_E2BIG;
                 break;
             }
-            argp = alloca((argc + 1) * sizeof(void *));
-            envp = alloca((envc + 1) * sizeof(void *));
+            argp = g_malloc0((argc + 1) * sizeof(void *));
+            envp = g_malloc0((envc + 1) * sizeof(void *));
 
             for (gp = guest_argp, q = argp; gp;
                   gp += sizeof(abi_ulong), q++) {
@@ -7862,6 +7862,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                     break;
                 unlock_user(*q, addr, 0);
             }
+
+            g_free(argp);
+            g_free(envp);
         }
         break;
     case TARGET_NR_chdir:
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

On 03/06/2017 01:17 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Arguments passed to execve(2) call from user program could
> be large, allocating stack memory for them via alloca(3) call
> would lead to bad behaviour. Use 'g_malloc0' to allocate memory
> for such arguments.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  linux-user/syscall.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Is this patch alone (without 1/2) sufficient to solve the problem?  If
so, then drop 1/2.

> 
> Update per: replace alloca() with g_malloc0()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 86a4a9c..404fb0b 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7800,8 +7800,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,

What version of qemu are you patching?  Line 7800 of current master is
nowhere near 'case TARGET_NR_execve:' (line 7899)

>                  ret = -TARGET_E2BIG;
>                  break;
>              }

and current master has 'goto efault' rather than directly setting ret at
this point.

> -            argp = alloca((argc + 1) * sizeof(void *));
> -            envp = alloca((envc + 1) * sizeof(void *));
> +            argp = g_malloc0((argc + 1) * sizeof(void *));
> +            envp = g_malloc0((envc + 1) * sizeof(void *));

Subject to a potential multiplication overflow.  I'd prefer:

g_new0(void *, argc + 1);

as that is guaranteed to not overflow.

>  
>              for (gp = guest_argp, q = argp; gp;
>                    gp += sizeof(abi_ulong), q++) {
> @@ -7862,6 +7862,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      break;
>                  unlock_user(*q, addr, 0);
>              }
> +
> +            g_free(argp);
> +            g_free(envp);
>          }
>          break;
>      case TARGET_NR_chdir:
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

On 03/06/2017 09:53 AM, Eric Blake wrote:
> On 03/06/2017 01:17 AM, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Arguments passed to execve(2) call from user program could
>> be large, allocating stack memory for them via alloca(3) call
>> would lead to bad behaviour. Use 'g_malloc0' to allocate memory
>> for such arguments.
>>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  linux-user/syscall.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Is this patch alone (without 1/2) sufficient to solve the problem?  If
> so, then drop 1/2.
> 
>>
>> Update per: replace alloca() with g_malloc0()
>>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 86a4a9c..404fb0b 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7800,8 +7800,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> 
> What version of qemu are you patching?  Line 7800 of current master is
> nowhere near 'case TARGET_NR_execve:' (line 7899)
> 
>>                  ret = -TARGET_E2BIG;
>>                  break;
>>              }
> 
> and current master has 'goto efault' rather than directly setting ret at
> this point.

Okay, I see that this context came from patch 1/2. Sorry for the noise
(I was trying to review this patch in isolation, since I've already
argued that 1/2 is probably not necessary).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[P J P]

+-- On Mon, 6 Mar 2017, Eric Blake wrote --+
| On 03/06/2017 01:17 AM, P J P wrote:
| > Arguments passed to execve(2) call from user program could
| > be large, allocating stack memory for them via alloca(3) call
| > would lead to bad behaviour. Use 'g_malloc0' to allocate memory
| > for such arguments.
| > 
| > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| > ---
| >  linux-user/syscall.c | 7 +++++--
| >  1 file changed, 5 insertions(+), 2 deletions(-)
| 
| Is this patch alone (without 1/2) sufficient to solve the problem?  If
| so, then drop 1/2.

  Yes, it seems to fix the issue. Still I think having ARG_MAX limit would be 
good, as system exec(3) routines too impose _SC_ARG_MAX limit. I'll send a 
revised patch with 'g_try_new' call instead of g_malloc0.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

On 03/06/2017 12:06 PM, P J P wrote:
> +-- On Mon, 6 Mar 2017, Eric Blake wrote --+
> | On 03/06/2017 01:17 AM, P J P wrote:
> | > Arguments passed to execve(2) call from user program could
> | > be large, allocating stack memory for them via alloca(3) call
> | > would lead to bad behaviour. Use 'g_malloc0' to allocate memory
> | > for such arguments.
> | > 
> | > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | > ---
> | >  linux-user/syscall.c | 7 +++++--
> | >  1 file changed, 5 insertions(+), 2 deletions(-)
> | 
> | Is this patch alone (without 1/2) sufficient to solve the problem?  If
> | so, then drop 1/2.
> 
>   Yes, it seems to fix the issue. Still I think having ARG_MAX limit would be 
> good, as system exec(3) routines too impose _SC_ARG_MAX limit. I'll send a 
> revised patch with 'g_try_new' call instead of g_malloc0.

If you impose any limit smaller than _SC_ARG_MAX, you are needlessly
limiting things.  Furthermore, _SC_ARG_MAX may not always be the same
value, depending on how the kernel was compiled.  So it's probably
asiest to just let execve() impose its own limits (and correctly report
errors to the caller when execve() fails), rather than trying to impose
limits yourself.

In short, the bug that you are fixing is not caused by the guest
requesting something beyond execve() limits, but caused by poor use of
alloca() leading to a stack overrun.  You only need to fix the bug (by
switching alloca() to heap allocation), not introduce additional ones
(by imposing arbitrary, and probably wrong, limits).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[P J P]

+-- On Mon, 6 Mar 2017, Eric Blake wrote --+
| If you impose any limit smaller than _SC_ARG_MAX, you are needlessly
| limiting things.  Furthermore, _SC_ARG_MAX may not always be the same
| value, depending on how the kernel was compiled.  So it's probably
| asiest to just let execve() impose its own limits (and correctly report
| errors to the caller when execve() fails), rather than trying to impose
| limits yourself.

  Okay.

| In short, the bug that you are fixing is not caused by the guest
| requesting something beyond execve() limits, but caused by poor use of
| alloca() leading to a stack overrun.

  Yes, true.

| You only need to fix the bug (by switching alloca() to heap allocation),

  Okay, I'll send just one revised patch.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v2 2/2] linux-user: allocate heap memory for execve arguments

[Peter Maydell]

On 6 March 2017 at 07:17, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Arguments passed to execve(2) call from user program could
> be large, allocating stack memory for them via alloca(3) call
> would lead to bad behaviour. Use 'g_malloc0' to allocate memory
> for such arguments.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  linux-user/syscall.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Update per: replace alloca() with g_malloc0()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00750.html
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 86a4a9c..404fb0b 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7800,8 +7800,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  ret = -TARGET_E2BIG;
>                  break;
>              }
> -            argp = alloca((argc + 1) * sizeof(void *));
> -            envp = alloca((envc + 1) * sizeof(void *));
> +            argp = g_malloc0((argc + 1) * sizeof(void *));
> +            envp = g_malloc0((envc + 1) * sizeof(void *));
>
>              for (gp = guest_argp, q = argp; gp;
>                    gp += sizeof(abi_ulong), q++) {
> @@ -7862,6 +7862,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                      break;
>                  unlock_user(*q, addr, 0);
>              }
> +
> +            g_free(argp);
> +            g_free(envp);
>          }
>          break;
>      case TARGET_NR_chdir:

Better to use the _try_ glib allocation functions and fail
the syscall on allocation failure, maybe?

thanks
-- PMM