* Re: [PATCH v3] linux-user: implement execveat
2022-11-03 17:32 [PATCH v3] linux-user: implement execveat Drew DeVault
@ 2022-11-03 19:54 ` Helge Deller
2022-11-04 9:53 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 7+ messages in thread
From: Helge Deller @ 2022-11-03 19:54 UTC (permalink / raw)
To: Drew DeVault, Laurent Vivier; +Cc: qemu-devel
On 11/3/22 18:32, Drew DeVault wrote:
> References: https://gitlab.com/qemu-project/qemu/-/issues/1007
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
> v2 => v3:
> - Rebase to address the is_proc_myself fix
> - Drop the ifdefs
>
> linux-user/syscall.c | 203 ++++++++++++++++++++++---------------------
> 1 file changed, 105 insertions(+), 98 deletions(-)
I'd suggest you add the missing strace part as well.
See in strace.list and compare what's done for execve.
Helge
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8402c1399d..38fbbbad6a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -689,7 +689,8 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
> #endif
> safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
> int, options, struct rusage *, rusage)
> -safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp)
> +safe_syscall5(int, execveat, int, dirfd, const char *, filename,
> + char **, argv, char **, envp, int, flags)
> #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
> defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6_time64)
> safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
> @@ -8349,6 +8350,106 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
> return safe_openat(dirfd, path(pathname), flags, mode);
> }
>
> +static int do_execveat(CPUArchState *cpu_env, int dirfd, abi_long pathname, abi_long guest_argp, abi_long guest_envp, int flags)
> +{
> + int ret;
> + char **argp, **envp;
> + int argc, envc;
> + abi_ulong gp;
> + abi_ulong addr;
> + char **q;
> + void *p;
> +
> + argc = 0;
> +
> + for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + argc++;
> + }
> + envc = 0;
> + for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + envc++;
> + }
> +
> + argp = g_new0(char *, argc + 1);
> + envp = g_new0(char *, envc + 1);
> +
> + for (gp = guest_argp, q = argp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + for (gp = guest_envp, q = envp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + /* Although execve() is not an interruptible syscall it is
> + * a special case where we must use the safe_syscall wrapper:
> + * if we allow a signal to happen before we make the host
> + * syscall then we will 'lose' it, because at the point of
> + * execve the process leaves QEMU's control. So we use the
> + * safe syscall wrapper to ensure that we either take the
> + * signal as a guest signal, or else it does not happen
> + * before the execve completes and makes it the other
> + * program's problem.
> + */
> + if (!(p = lock_user_string(pathname)))
> + goto execve_efault;
> +
> + if (is_proc_myself(p, "exe")) {
> + ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags));
> + } else {
> + ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags));
> + }
> +
> + unlock_user(p, pathname, 0);
> +
> + goto execve_end;
> +
> +execve_efault:
> + ret = -TARGET_EFAULT;
> +
> +execve_end:
> + for (gp = guest_argp, q = argp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> + for (gp = guest_envp, q = envp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> +
> + g_free(argp);
> + g_free(envp);
> + return ret;
> +}
> +
> #define TIMER_MAGIC 0x0caf0000
> #define TIMER_MAGIC_MASK 0xffff0000
>
> @@ -8846,104 +8947,10 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> unlock_user(p, arg2, 0);
> return ret;
> #endif
> + case TARGET_NR_execveat:
> + return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5);
> case TARGET_NR_execve:
> - {
> - char **argp, **envp;
> - int argc, envc;
> - abi_ulong gp;
> - abi_ulong guest_argp;
> - abi_ulong guest_envp;
> - abi_ulong addr;
> - char **q;
> -
> - argc = 0;
> - guest_argp = arg2;
> - for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - argc++;
> - }
> - envc = 0;
> - guest_envp = arg3;
> - for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - envc++;
> - }
> -
> - argp = g_new0(char *, argc + 1);
> - envp = g_new0(char *, envc + 1);
> -
> - for (gp = guest_argp, q = argp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - for (gp = guest_envp, q = envp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - if (!(p = lock_user_string(arg1)))
> - goto execve_efault;
> - /* Although execve() is not an interruptible syscall it is
> - * a special case where we must use the safe_syscall wrapper:
> - * if we allow a signal to happen before we make the host
> - * syscall then we will 'lose' it, because at the point of
> - * execve the process leaves QEMU's control. So we use the
> - * safe syscall wrapper to ensure that we either take the
> - * signal as a guest signal, or else it does not happen
> - * before the execve completes and makes it the other
> - * program's problem.
> - */
> - if (is_proc_myself(p, "exe")) {
> - ret = get_errno(safe_execve(exec_path, argp, envp));
> - } else {
> - ret = get_errno(safe_execve(p, argp, envp));
> - }
> - unlock_user(p, arg1, 0);
> -
> - goto execve_end;
> -
> - execve_efault:
> - ret = -TARGET_EFAULT;
> -
> - execve_end:
> - for (gp = guest_argp, q = argp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> - for (gp = guest_envp, q = envp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> -
> - g_free(argp);
> - g_free(envp);
> - }
> - return ret;
> + return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0);
> case TARGET_NR_chdir:
> if (!(p = lock_user_string(arg1)))
> return -TARGET_EFAULT;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] linux-user: implement execveat
2022-11-03 17:32 [PATCH v3] linux-user: implement execveat Drew DeVault
2022-11-03 19:54 ` Helge Deller
@ 2022-11-04 9:53 ` Philippe Mathieu-Daudé
2022-11-04 9:55 ` Drew DeVault
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-04 9:53 UTC (permalink / raw)
To: Drew DeVault, Laurent Vivier; +Cc: qemu-devel, Helge Deller
Hi Drew,
On 3/11/22 18:32, Drew DeVault wrote:
> References: https://gitlab.com/qemu-project/qemu/-/issues/1007
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
> v2 => v3:
> - Rebase to address the is_proc_myself fix
> - Drop the ifdefs
>
> linux-user/syscall.c | 203 ++++++++++++++++++++++---------------------
> 1 file changed, 105 insertions(+), 98 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8402c1399d..38fbbbad6a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -689,7 +689,8 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
> #endif
> safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
> int, options, struct rusage *, rusage)
> -safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp)
> +safe_syscall5(int, execveat, int, dirfd, const char *, filename,
> + char **, argv, char **, envp, int, flags)
> #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
> defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6_time64)
> safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
> @@ -8349,6 +8350,106 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
> return safe_openat(dirfd, path(pathname), flags, mode);
> }
>
> +static int do_execveat(CPUArchState *cpu_env, int dirfd, abi_long pathname, abi_long guest_argp, abi_long guest_envp, int flags)
> +{
> + int ret;
> + char **argp, **envp;
> + int argc, envc;
> + abi_ulong gp;
> + abi_ulong addr;
> + char **q;
> + void *p;
> +
> + argc = 0;
> +
> + for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + argc++;
> + }
> + envc = 0;
> + for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + envc++;
> + }
> +
> + argp = g_new0(char *, argc + 1);
> + envp = g_new0(char *, envc + 1);
> +
> + for (gp = guest_argp, q = argp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + for (gp = guest_envp, q = envp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + /* Although execve() is not an interruptible syscall it is
> + * a special case where we must use the safe_syscall wrapper:
> + * if we allow a signal to happen before we make the host
> + * syscall then we will 'lose' it, because at the point of
> + * execve the process leaves QEMU's control. So we use the
> + * safe syscall wrapper to ensure that we either take the
> + * signal as a guest signal, or else it does not happen
> + * before the execve completes and makes it the other
> + * program's problem.
> + */
> + if (!(p = lock_user_string(pathname)))
> + goto execve_efault;
> +
> + if (is_proc_myself(p, "exe")) {
> + ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags));
> + } else {
> + ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags));
> + }
> +
> + unlock_user(p, pathname, 0);
> +
> + goto execve_end;
> +
> +execve_efault:
> + ret = -TARGET_EFAULT;
> +
> +execve_end:
> + for (gp = guest_argp, q = argp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> + for (gp = guest_envp, q = envp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> +
> + g_free(argp);
> + g_free(envp);
> + return ret;
> +}
> +
> #define TIMER_MAGIC 0x0caf0000
> #define TIMER_MAGIC_MASK 0xffff0000
>
> @@ -8846,104 +8947,10 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> unlock_user(p, arg2, 0);
> return ret;
> #endif
> + case TARGET_NR_execveat:
> + return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5);
> case TARGET_NR_execve:
> - {
> - char **argp, **envp;
> - int argc, envc;
> - abi_ulong gp;
> - abi_ulong guest_argp;
> - abi_ulong guest_envp;
> - abi_ulong addr;
> - char **q;
> -
> - argc = 0;
> - guest_argp = arg2;
> - for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - argc++;
> - }
> - envc = 0;
> - guest_envp = arg3;
> - for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - envc++;
> - }
> -
> - argp = g_new0(char *, argc + 1);
> - envp = g_new0(char *, envc + 1);
> -
> - for (gp = guest_argp, q = argp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - for (gp = guest_envp, q = envp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - if (!(p = lock_user_string(arg1)))
> - goto execve_efault;
> - /* Although execve() is not an interruptible syscall it is
> - * a special case where we must use the safe_syscall wrapper:
> - * if we allow a signal to happen before we make the host
> - * syscall then we will 'lose' it, because at the point of
> - * execve the process leaves QEMU's control. So we use the
> - * safe syscall wrapper to ensure that we either take the
> - * signal as a guest signal, or else it does not happen
> - * before the execve completes and makes it the other
> - * program's problem.
> - */
> - if (is_proc_myself(p, "exe")) {
> - ret = get_errno(safe_execve(exec_path, argp, envp));
> - } else {
> - ret = get_errno(safe_execve(p, argp, envp));
> - }
> - unlock_user(p, arg1, 0);
> -
> - goto execve_end;
> -
> - execve_efault:
> - ret = -TARGET_EFAULT;
> -
> - execve_end:
> - for (gp = guest_argp, q = argp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> - for (gp = guest_envp, q = envp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> -
> - g_free(argp);
> - g_free(envp);
> - }
> - return ret;
> + return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0);
> case TARGET_NR_chdir:
> if (!(p = lock_user_string(arg1)))
> return -TARGET_EFAULT;
Splitting this big patch would ease review:
1/ Replace safe_execve() by safe_execveat()
-safe_execve(exec_path, argp, envp))
+safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
2/ Extract do_execve()
3/ Convert do_execve() to do_execveat() adding dirfd/flags args
4/ Add TARGET_NR_execveat case
Thanks,
Phil.
^ permalink raw reply [flat|nested] 7+ messages in thread