From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: omerg681@gmail.com, qemu-arm@nongnu.org,
Riku Voipio <riku.voipio@iki.fi>,
qemu-devel@nongnu.org, Laurent Vivier <laurent@vivier.eu>
Subject: Re: [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers
Date: Tue, 21 Apr 2020 09:57:16 +0200 [thread overview]
Message-ID: <20200421075715.GH2669@toto> (raw)
In-Reply-To: <20200420212206.12776-5-peter.maydell@linaro.org>
On Mon, Apr 20, 2020 at 10:22:06PM +0100, Peter Maydell wrote:
> Our code to identify syscall numbers has some issues:
> * for Thumb mode, we never need the immediate value from the insn,
> but we always read it anyway
> * bad immediate values in the svc insn should cause a SIGILL, but we
> were abort()ing instead (via "goto error")
>
> We can fix both these things by refactoring the code that identifies
> the syscall number to more closely follow the kernel COMPAT_OABI code:
> * for Thumb it is always r7
> * for Arm, if the immediate value is 0, then this is an EABI call
> with the syscall number in r7
> * otherwise, we XOR the immediate value with 0x900000
> (ARM_SYSCALL_BASE for QEMU; __NR_OABI_SYSCALL_BASE in the kernel),
> which converts valid syscall immediates into the desired value,
> and puts all invalid immediates in the range 0x100000 or above
> * then we can just let the existing "value too large, deliver
> SIGILL" case handle invalid numbers, and drop the 'goto error'
I guess the -2 vs -4 issue has propagated into this patch but with that fixed:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> You might prefer to read this patch with an "ignore whitespace
> changes" diff, as a big chunk of code is no longer inside an if()
> and got re-indented out one level.
> ---
> linux-user/arm/cpu_loop.c | 143 ++++++++++++++++++++------------------
> 1 file changed, 77 insertions(+), 66 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index f042108b0be..eeb042829e2 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -299,85 +299,96 @@ void cpu_loop(CPUARMState *env)
> env->eabi = 1;
> /* system call */
> if (env->thumb) {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u16(insn, env->regs[15] - 2, env);
> - n = insn & 0xff;
> + /* Thumb is always EABI style with syscall number in r7 */
> + n = env->regs[7];
> } else {
> + /*
> + * Equivalent of kernel CONFIG_OABI_COMPAT: read the
> + * Arm SVC insn to extract the immediate, which is the
> + * syscall number in OABI.
> + */
> /* FIXME - what to do if get_user() fails? */
> get_user_code_u32(insn, env->regs[15] - 4, env);
> n = insn & 0xffffff;
> - }
> -
> - if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> - /* linux syscall */
> - if (env->thumb || n == 0) {
> + if (n == 0) {
> + /* zero immediate: EABI, syscall number in r7 */
> n = env->regs[7];
> } else {
> - n -= ARM_SYSCALL_BASE;
> + /*
> + * This XOR matches the kernel code: an immediate
> + * in the valid range (0x900000 .. 0x9fffff) is
> + * converted into the correct EABI-style syscall
> + * number; invalid immediates end up as values
> + * > 0xfffff and are handled below as out-of-range.
> + */
> + n ^= ARM_SYSCALL_BASE;
> env->eabi = 0;
> }
> - if ( n > ARM_NR_BASE) {
> - switch (n) {
> - case ARM_NR_cacheflush:
> - /* nop */
> - break;
> - case ARM_NR_set_tls:
> - cpu_set_tls(env, env->regs[0]);
> - env->regs[0] = 0;
> - break;
> - case ARM_NR_breakpoint:
> - env->regs[15] -= env->thumb ? 2 : 4;
> - goto excp_debug;
> - case ARM_NR_get_tls:
> - env->regs[0] = cpu_get_tls(env);
> - break;
> - default:
> - if (n < 0xf0800) {
> - /*
> - * Syscalls 0xf0000..0xf07ff (or 0x9f0000..
> - * 0x9f07ff in OABI numbering) are defined
> - * to return -ENOSYS rather than raising
> - * SIGILL. Note that we have already
> - * removed the 0x900000 prefix.
> - */
> - qemu_log_mask(LOG_UNIMP,
> - "qemu: Unsupported ARM syscall: 0x%x\n",
> - n);
> - env->regs[0] = -TARGET_ENOSYS;
> + }
> +
> + if (n > ARM_NR_BASE) {
> + switch (n) {
> + case ARM_NR_cacheflush:
> + /* nop */
> + break;
> + case ARM_NR_set_tls:
> + cpu_set_tls(env, env->regs[0]);
> + env->regs[0] = 0;
> + break;
> + case ARM_NR_breakpoint:
> + env->regs[15] -= env->thumb ? 2 : 4;
> + goto excp_debug;
> + case ARM_NR_get_tls:
> + env->regs[0] = cpu_get_tls(env);
> + break;
> + default:
> + if (n < 0xf0800) {
> + /*
> + * Syscalls 0xf0000..0xf07ff (or 0x9f0000..
> + * 0x9f07ff in OABI numbering) are defined
> + * to return -ENOSYS rather than raising
> + * SIGILL. Note that we have already
> + * removed the 0x900000 prefix.
> + */
> + qemu_log_mask(LOG_UNIMP,
> + "qemu: Unsupported ARM syscall: 0x%x\n",
> + n);
> + env->regs[0] = -TARGET_ENOSYS;
> + } else {
> + /*
> + * Otherwise SIGILL. This includes any SWI with
> + * immediate not originally 0x9fxxxx, because
> + * of the earlier XOR.
> + */
> + info.si_signo = TARGET_SIGILL;
> + info.si_errno = 0;
> + info.si_code = TARGET_ILL_ILLTRP;
> + info._sifields._sigfault._addr = env->regs[15];
> + if (env->thumb) {
> + info._sifields._sigfault._addr -= 2;
> } else {
> - /* Otherwise SIGILL */
> - info.si_signo = TARGET_SIGILL;
> - info.si_errno = 0;
> - info.si_code = TARGET_ILL_ILLTRP;
> - info._sifields._sigfault._addr = env->regs[15];
> - if (env->thumb) {
> - info._sifields._sigfault._addr -= 2;
> - } else {
> - info._sifields._sigfault._addr -= 2;
> - }
> - queue_signal(env, info.si_signo,
> - QEMU_SI_FAULT, &info);
> + info._sifields._sigfault._addr -= 2;
> }
> - break;
> - }
> - } else {
> - ret = do_syscall(env,
> - n,
> - env->regs[0],
> - env->regs[1],
> - env->regs[2],
> - env->regs[3],
> - env->regs[4],
> - env->regs[5],
> - 0, 0);
> - if (ret == -TARGET_ERESTARTSYS) {
> - env->regs[15] -= env->thumb ? 2 : 4;
> - } else if (ret != -TARGET_QEMU_ESIGRETURN) {
> - env->regs[0] = ret;
> + queue_signal(env, info.si_signo,
> + QEMU_SI_FAULT, &info);
> }
> + break;
> }
> } else {
> - goto error;
> + ret = do_syscall(env,
> + n,
> + env->regs[0],
> + env->regs[1],
> + env->regs[2],
> + env->regs[3],
> + env->regs[4],
> + env->regs[5],
> + 0, 0);
> + if (ret == -TARGET_ERESTARTSYS) {
> + env->regs[15] -= env->thumb ? 2 : 4;
> + } else if (ret != -TARGET_QEMU_ESIGRETURN) {
> + env->regs[0] = ret;
> + }
> }
> }
> break;
> --
> 2.20.1
>
>
next prev parent reply other threads:[~2020-04-21 7:56 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-20 21:22 [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
2020-04-20 21:22 ` [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall Peter Maydell
2020-04-21 7:48 ` Edgar E. Iglesias
2020-04-21 7:48 ` Philippe Mathieu-Daudé
2020-04-21 8:48 ` Peter Maydell
2020-04-20 21:22 ` [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling Peter Maydell
2020-04-21 7:39 ` Philippe Mathieu-Daudé
2020-04-21 7:49 ` Edgar E. Iglesias
2020-04-20 21:22 ` [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly Peter Maydell
2020-04-21 7:36 ` Philippe Mathieu-Daudé
2020-04-21 7:44 ` Edgar E. Iglesias
2020-04-21 7:51 ` Philippe Mathieu-Daudé
2020-04-21 8:49 ` Peter Maydell
2020-04-21 9:31 ` Aleksandar Markovic
2020-04-21 9:34 ` Peter Maydell
2020-04-20 21:22 ` [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers Peter Maydell
2020-04-21 7:57 ` Edgar E. Iglesias [this message]
2020-05-12 12:43 ` [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
2020-05-18 15:00 ` Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200421075715.GH2669@toto \
--to=edgar.iglesias@gmail.com \
--cc=laurent@vivier.eu \
--cc=omerg681@gmail.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=riku.voipio@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.