| Message ID | 20170306071721.26708-3-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: >
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
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).
+-- 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
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).
+-- 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
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: