* [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall
2020-04-20 21:22 [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
@ 2020-04-20 21:22 ` Peter Maydell
2020-04-21 7:48 ` Edgar E. Iglesias
2020-04-21 7:48 ` Philippe Mathieu-Daudé
2020-04-20 21:22 ` [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling Peter Maydell
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-20 21:22 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
In linux-user/arm/cpu-loop.c we incorrectly treat EXCP_BKPT similarly
to EXCP_SWI, which means that if the guest executes a BKPT insn then
QEMU will perform a syscall for it (which syscall depends on what
value happens to be in r7...). The correct behaviour is that the
guest process should take a SIGTRAP.
This code has been like this (more or less) since commit
06c949e62a098f in 2006 which added BKPT in the first place. This is
probably because at the time the same code path was used to handle
both Linux syscalls and semihosting calls, and (on M profile) BKPT
with a suitable magic number is used for semihosting calls. But
these days we've moved handling of semihosting out to an entirely
different codepath, so we can fix this bug by simply removing this
handling of EXCP_BKPT and instead making it deliver a SIGTRAP like
EXCP_DEBUG (as we do already on aarch64).
Reported-by: <omerg681@gmail.com>
Fixes: https://bugs.launchpad.net/qemu/+bug/1873898
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/arm/cpu_loop.c | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index cf618daa1ca..82d0dd3c312 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -295,32 +295,17 @@ void cpu_loop(CPUARMState *env)
}
break;
case EXCP_SWI:
- case EXCP_BKPT:
{
env->eabi = 1;
/* system call */
- if (trapnr == EXCP_BKPT) {
- if (env->thumb) {
- /* FIXME - what to do if get_user() fails? */
- get_user_code_u16(insn, env->regs[15], env);
- n = insn & 0xff;
- env->regs[15] += 2;
- } else {
- /* FIXME - what to do if get_user() fails? */
- get_user_code_u32(insn, env->regs[15], env);
- n = (insn & 0xf) | ((insn >> 4) & 0xff0);
- env->regs[15] += 4;
- }
+ if (env->thumb) {
+ /* FIXME - what to do if get_user() fails? */
+ get_user_code_u16(insn, env->regs[15] - 2, env);
+ n = insn & 0xff;
} else {
- if (env->thumb) {
- /* FIXME - what to do if get_user() fails? */
- get_user_code_u16(insn, env->regs[15] - 2, env);
- n = insn & 0xff;
- } else {
- /* FIXME - what to do if get_user() fails? */
- get_user_code_u32(insn, env->regs[15] - 4, env);
- n = insn & 0xffffff;
- }
+ /* FIXME - what to do if get_user() fails? */
+ get_user_code_u32(insn, env->regs[15] - 4, env);
+ n = insn & 0xffffff;
}
if (n == ARM_NR_cacheflush) {
@@ -396,6 +381,7 @@ void cpu_loop(CPUARMState *env)
}
break;
case EXCP_DEBUG:
+ case EXCP_BKPT:
excp_debug:
info.si_signo = TARGET_SIGTRAP;
info.si_errno = 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall
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é
1 sibling, 0 replies; 19+ messages in thread
From: Edgar E. Iglesias @ 2020-04-21 7:48 UTC (permalink / raw)
To: Peter Maydell; +Cc: omerg681, qemu-arm, Riku Voipio, qemu-devel, Laurent Vivier
On Mon, Apr 20, 2020 at 10:22:03PM +0100, Peter Maydell wrote:
> In linux-user/arm/cpu-loop.c we incorrectly treat EXCP_BKPT similarly
> to EXCP_SWI, which means that if the guest executes a BKPT insn then
> QEMU will perform a syscall for it (which syscall depends on what
> value happens to be in r7...). The correct behaviour is that the
> guest process should take a SIGTRAP.
>
> This code has been like this (more or less) since commit
> 06c949e62a098f in 2006 which added BKPT in the first place. This is
> probably because at the time the same code path was used to handle
> both Linux syscalls and semihosting calls, and (on M profile) BKPT
> with a suitable magic number is used for semihosting calls. But
> these days we've moved handling of semihosting out to an entirely
> different codepath, so we can fix this bug by simply removing this
> handling of EXCP_BKPT and instead making it deliver a SIGTRAP like
> EXCP_DEBUG (as we do already on aarch64).
>
> Reported-by: <omerg681@gmail.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1873898
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> linux-user/arm/cpu_loop.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index cf618daa1ca..82d0dd3c312 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -295,32 +295,17 @@ void cpu_loop(CPUARMState *env)
> }
> break;
> case EXCP_SWI:
> - case EXCP_BKPT:
> {
> env->eabi = 1;
> /* system call */
> - if (trapnr == EXCP_BKPT) {
> - if (env->thumb) {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u16(insn, env->regs[15], env);
> - n = insn & 0xff;
> - env->regs[15] += 2;
> - } else {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u32(insn, env->regs[15], env);
> - n = (insn & 0xf) | ((insn >> 4) & 0xff0);
> - env->regs[15] += 4;
> - }
> + if (env->thumb) {
> + /* FIXME - what to do if get_user() fails? */
> + get_user_code_u16(insn, env->regs[15] - 2, env);
> + n = insn & 0xff;
> } else {
> - if (env->thumb) {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u16(insn, env->regs[15] - 2, env);
> - n = insn & 0xff;
> - } else {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u32(insn, env->regs[15] - 4, env);
> - n = insn & 0xffffff;
> - }
> + /* FIXME - what to do if get_user() fails? */
> + get_user_code_u32(insn, env->regs[15] - 4, env);
> + n = insn & 0xffffff;
> }
>
> if (n == ARM_NR_cacheflush) {
> @@ -396,6 +381,7 @@ void cpu_loop(CPUARMState *env)
> }
> break;
> case EXCP_DEBUG:
> + case EXCP_BKPT:
> excp_debug:
> info.si_signo = TARGET_SIGTRAP;
> info.si_errno = 0;
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall
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
1 sibling, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-21 7:48 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
On 4/20/20 11:22 PM, Peter Maydell wrote:
> In linux-user/arm/cpu-loop.c we incorrectly treat EXCP_BKPT similarly
> to EXCP_SWI, which means that if the guest executes a BKPT insn then
> QEMU will perform a syscall for it (which syscall depends on what
> value happens to be in r7...). The correct behaviour is that the
> guest process should take a SIGTRAP.
>
> This code has been like this (more or less) since commit
> 06c949e62a098f in 2006 which added BKPT in the first place. This is
> probably because at the time the same code path was used to handle
> both Linux syscalls and semihosting calls, and (on M profile) BKPT
> with a suitable magic number is used for semihosting calls. But
> these days we've moved handling of semihosting out to an entirely
> different codepath, so we can fix this bug by simply removing this
> handling of EXCP_BKPT and instead making it deliver a SIGTRAP like
> EXCP_DEBUG (as we do already on aarch64).
>
> Reported-by: <omerg681@gmail.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1873898
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index cf618daa1ca..82d0dd3c312 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -295,32 +295,17 @@ void cpu_loop(CPUARMState *env)
> }
> break;
> case EXCP_SWI:
> - case EXCP_BKPT:
> {
> env->eabi = 1;
> /* system call */
> - if (trapnr == EXCP_BKPT) {
> - if (env->thumb) {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u16(insn, env->regs[15], env);
> - n = insn & 0xff;
> - env->regs[15] += 2;
> - } else {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u32(insn, env->regs[15], env);
> - n = (insn & 0xf) | ((insn >> 4) & 0xff0);
> - env->regs[15] += 4;
> - }
> + if (env->thumb) {
> + /* FIXME - what to do if get_user() fails? */
> + get_user_code_u16(insn, env->regs[15] - 2, env);
> + n = insn & 0xff;
> } else {
> - if (env->thumb) {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u16(insn, env->regs[15] - 2, env);
> - n = insn & 0xff;
> - } else {
> - /* FIXME - what to do if get_user() fails? */
> - get_user_code_u32(insn, env->regs[15] - 4, env);
> - n = insn & 0xffffff;
> - }
> + /* FIXME - what to do if get_user() fails? */
> + get_user_code_u32(insn, env->regs[15] - 4, env);
> + n = insn & 0xffffff;
> }
I couldn't find a git-diff option to display this change in an obvious way.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> if (n == ARM_NR_cacheflush) {
> @@ -396,6 +381,7 @@ void cpu_loop(CPUARMState *env)
> }
> break;
> case EXCP_DEBUG:
> + case EXCP_BKPT:
> excp_debug:
> info.si_signo = TARGET_SIGTRAP;
> info.si_errno = 0;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] linux-user/arm: BKPT should cause SIGTRAP, not be a syscall
2020-04-21 7:48 ` Philippe Mathieu-Daudé
@ 2020-04-21 8:48 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-21 8:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: omerg681, qemu-arm, Riku Voipio, QEMU Developers, Laurent Vivier
On Tue, 21 Apr 2020 at 08:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I couldn't find a git-diff option to display this change in an obvious way.
Yeah, as a human you can say "it would be easier to read
if the '} else {' line was not treated as unchanged", but the
automatic diff output isn't totally awful. Sometimes you
do just have to look at the resulting code...
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling
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-20 21:22 ` 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
` (2 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-20 21:22 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
We incorrectly treat SVC 0xf0002 as a cacheflush request (which is a
NOP for QEMU). This is the wrong syscall number, because in the
svc-immediate OABI syscall numbers are all offset by the
ARM_SYSCALL_BASE value and so the correct insn is SVC 0x9f0002.
(This is handled further down in the code with the other Arm-specific
syscalls like NR_breakpoint.)
When this code was initially added in commit 6f1f31c069b20611 in
2004, ARM_NR_cacheflush was defined as (ARM_SYSCALL_BASE + 0xf0000 + 2)
so the value in the comparison took account of the extra 0x900000
offset. In commit fbb4a2e371f2fa7 in 2008, the ARM_SYSCALL_BASE
was removed from the definition of ARM_NR_cacheflush and handling
for this group of syscalls was added below the point where we subtract
ARM_SYSCALL_BASE from the SVC immediate value. However that commit
forgot to remove the now-obsolete earlier handling code.
Remove the spurious ARM_NR_cacheflush condition.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/arm/cpu_loop.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 82d0dd3c312..025887d6b86 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -308,9 +308,7 @@ void cpu_loop(CPUARMState *env)
n = insn & 0xffffff;
}
- if (n == ARM_NR_cacheflush) {
- /* nop */
- } else if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
+ if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
/* linux syscall */
if (env->thumb || n == 0) {
n = env->regs[7];
--
2.20.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling
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
1 sibling, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-21 7:39 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
On 4/20/20 11:22 PM, Peter Maydell wrote:
> We incorrectly treat SVC 0xf0002 as a cacheflush request (which is a
> NOP for QEMU). This is the wrong syscall number, because in the
> svc-immediate OABI syscall numbers are all offset by the
> ARM_SYSCALL_BASE value and so the correct insn is SVC 0x9f0002.
> (This is handled further down in the code with the other Arm-specific
> syscalls like NR_breakpoint.)
>
> When this code was initially added in commit 6f1f31c069b20611 in
> 2004, ARM_NR_cacheflush was defined as (ARM_SYSCALL_BASE + 0xf0000 + 2)
> so the value in the comparison took account of the extra 0x900000
> offset. In commit fbb4a2e371f2fa7 in 2008, the ARM_SYSCALL_BASE
> was removed from the definition of ARM_NR_cacheflush and handling
> for this group of syscalls was added below the point where we subtract
> ARM_SYSCALL_BASE from the SVC immediate value. However that commit
> forgot to remove the now-obsolete earlier handling code.
I imagine you wrote this patch wearing an archeologist hat =)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Remove the spurious ARM_NR_cacheflush condition.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 82d0dd3c312..025887d6b86 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -308,9 +308,7 @@ void cpu_loop(CPUARMState *env)
> n = insn & 0xffffff;
> }
>
> - if (n == ARM_NR_cacheflush) {
> - /* nop */
> - } else if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> + if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> /* linux syscall */
> if (env->thumb || n == 0) {
> n = env->regs[7];
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling
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
1 sibling, 0 replies; 19+ messages in thread
From: Edgar E. Iglesias @ 2020-04-21 7:49 UTC (permalink / raw)
To: Peter Maydell; +Cc: omerg681, qemu-arm, Riku Voipio, qemu-devel, Laurent Vivier
On Mon, Apr 20, 2020 at 10:22:04PM +0100, Peter Maydell wrote:
> We incorrectly treat SVC 0xf0002 as a cacheflush request (which is a
> NOP for QEMU). This is the wrong syscall number, because in the
> svc-immediate OABI syscall numbers are all offset by the
> ARM_SYSCALL_BASE value and so the correct insn is SVC 0x9f0002.
> (This is handled further down in the code with the other Arm-specific
> syscalls like NR_breakpoint.)
>
> When this code was initially added in commit 6f1f31c069b20611 in
> 2004, ARM_NR_cacheflush was defined as (ARM_SYSCALL_BASE + 0xf0000 + 2)
> so the value in the comparison took account of the extra 0x900000
> offset. In commit fbb4a2e371f2fa7 in 2008, the ARM_SYSCALL_BASE
> was removed from the definition of ARM_NR_cacheflush and handling
> for this group of syscalls was added below the point where we subtract
> ARM_SYSCALL_BASE from the SVC immediate value. However that commit
> forgot to remove the now-obsolete earlier handling code.
>
> Remove the spurious ARM_NR_cacheflush condition.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 82d0dd3c312..025887d6b86 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -308,9 +308,7 @@ void cpu_loop(CPUARMState *env)
> n = insn & 0xffffff;
> }
>
> - if (n == ARM_NR_cacheflush) {
> - /* nop */
> - } else if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> + if (n == 0 || n >= ARM_SYSCALL_BASE || env->thumb) {
> /* linux syscall */
> if (env->thumb || n == 0) {
> n = env->regs[7];
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
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-20 21:22 ` [PATCH 2/4] linux-user/arm: Remove bogus SVC 0xf0002 handling Peter Maydell
@ 2020-04-20 21:22 ` Peter Maydell
2020-04-21 7:36 ` Philippe Mathieu-Daudé
` (2 more replies)
2020-04-20 21:22 ` [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers Peter Maydell
2020-05-12 12:43 ` [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
4 siblings, 3 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-20 21:22 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
The kernel has different handling for syscalls with invalid
numbers that are in the "arm-specific" range 0x9f0000 and up:
* 0x9f0000..0x9f07ff return -ENOSYS if not implemented
* other out of range syscalls cause a SIGILL
(see the kernel's arch/arm/kernel/traps.c:arm_syscall())
Implement this distinction. (Note that our code doesn't look
quite like the kernel's, because we have removed the
0x900000 prefix by this point, whereas the kernel retains
it in arm_syscall().)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
linux-user/arm/cpu_loop.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 025887d6b86..f042108b0be 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -332,10 +332,32 @@ void cpu_loop(CPUARMState *env)
env->regs[0] = cpu_get_tls(env);
break;
default:
- qemu_log_mask(LOG_UNIMP,
- "qemu: Unsupported ARM syscall: 0x%x\n",
- n);
- env->regs[0] = -TARGET_ENOSYS;
+ 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 */
+ 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);
+ }
break;
}
} else {
--
2.20.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
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 9:31 ` Aleksandar Markovic
2 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-21 7:36 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
On 4/20/20 11:22 PM, Peter Maydell wrote:
> The kernel has different handling for syscalls with invalid
> numbers that are in the "arm-specific" range 0x9f0000 and up:
> * 0x9f0000..0x9f07ff return -ENOSYS if not implemented
> * other out of range syscalls cause a SIGILL
> (see the kernel's arch/arm/kernel/traps.c:arm_syscall())
>
> Implement this distinction. (Note that our code doesn't look
> quite like the kernel's, because we have removed the
> 0x900000 prefix by this point, whereas the kernel retains
> it in arm_syscall().)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 025887d6b86..f042108b0be 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -332,10 +332,32 @@ void cpu_loop(CPUARMState *env)
> env->regs[0] = cpu_get_tls(env);
> break;
> default:
> - qemu_log_mask(LOG_UNIMP,
> - "qemu: Unsupported ARM syscall: 0x%x\n",
> - n);
> - env->regs[0] = -TARGET_ENOSYS;
> + 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 */
> + 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);
> + }
> break;
> }
> } else {
>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
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
2 siblings, 2 replies; 19+ messages in thread
From: Edgar E. Iglesias @ 2020-04-21 7:44 UTC (permalink / raw)
To: Peter Maydell; +Cc: omerg681, qemu-arm, Riku Voipio, qemu-devel, Laurent Vivier
On Mon, Apr 20, 2020 at 10:22:05PM +0100, Peter Maydell wrote:
> The kernel has different handling for syscalls with invalid
> numbers that are in the "arm-specific" range 0x9f0000 and up:
> * 0x9f0000..0x9f07ff return -ENOSYS if not implemented
> * other out of range syscalls cause a SIGILL
> (see the kernel's arch/arm/kernel/traps.c:arm_syscall())
>
> Implement this distinction. (Note that our code doesn't look
> quite like the kernel's, because we have removed the
> 0x900000 prefix by this point, whereas the kernel retains
> it in arm_syscall().)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 025887d6b86..f042108b0be 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -332,10 +332,32 @@ void cpu_loop(CPUARMState *env)
> env->regs[0] = cpu_get_tls(env);
> break;
> default:
> - qemu_log_mask(LOG_UNIMP,
> - "qemu: Unsupported ARM syscall: 0x%x\n",
> - n);
> - env->regs[0] = -TARGET_ENOSYS;
> + 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 */
> + 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;
> + }
Am I missing some detail or are both branches of the if-else doing the
same thing?
Cheers,
Edgar
> + queue_signal(env, info.si_signo,
> + QEMU_SI_FAULT, &info);
> + }
> break;
> }
> } else {
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
2020-04-21 7:44 ` Edgar E. Iglesias
@ 2020-04-21 7:51 ` Philippe Mathieu-Daudé
2020-04-21 8:49 ` Peter Maydell
1 sibling, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-21 7:51 UTC (permalink / raw)
To: Edgar E. Iglesias, Peter Maydell
Cc: omerg681, qemu-arm, Riku Voipio, qemu-devel, Laurent Vivier
On 4/21/20 9:44 AM, Edgar E. Iglesias wrote:
> On Mon, Apr 20, 2020 at 10:22:05PM +0100, Peter Maydell wrote:
>> The kernel has different handling for syscalls with invalid
>> numbers that are in the "arm-specific" range 0x9f0000 and up:
>> * 0x9f0000..0x9f07ff return -ENOSYS if not implemented
>> * other out of range syscalls cause a SIGILL
>> (see the kernel's arch/arm/kernel/traps.c:arm_syscall())
>>
>> Implement this distinction. (Note that our code doesn't look
>> quite like the kernel's, because we have removed the
>> 0x900000 prefix by this point, whereas the kernel retains
>> it in arm_syscall().)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> linux-user/arm/cpu_loop.c | 30 ++++++++++++++++++++++++++----
>> 1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
>> index 025887d6b86..f042108b0be 100644
>> --- a/linux-user/arm/cpu_loop.c
>> +++ b/linux-user/arm/cpu_loop.c
>> @@ -332,10 +332,32 @@ void cpu_loop(CPUARMState *env)
>> env->regs[0] = cpu_get_tls(env);
>> break;
>> default:
>> - qemu_log_mask(LOG_UNIMP,
>> - "qemu: Unsupported ARM syscall: 0x%x\n",
>> - n);
>> - env->regs[0] = -TARGET_ENOSYS;
>> + 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 */
>> + 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;
>> + }
>
>
> Am I missing some detail or are both branches of the if-else doing the
> same thing?
Oops good catch. R-b stands using '-= 4' on 2nd line.
>
> Cheers,
> Edgar
>
>
>
>> + queue_signal(env, info.si_signo,
>> + QEMU_SI_FAULT, &info);
>> + }
>> break;
>> }
>> } else {
>> --
>> 2.20.1
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
2020-04-21 7:44 ` Edgar E. Iglesias
2020-04-21 7:51 ` Philippe Mathieu-Daudé
@ 2020-04-21 8:49 ` Peter Maydell
1 sibling, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-21 8:49 UTC (permalink / raw)
To: Edgar E. Iglesias
Cc: omerg681, qemu-arm, Riku Voipio, QEMU Developers, Laurent Vivier
On Tue, 21 Apr 2020 at 08:42, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> On Mon, Apr 20, 2020 at 10:22:05PM +0100, Peter Maydell wrote:
> > + if (env->thumb) {
> > + info._sifields._sigfault._addr -= 2;
> > + } else {
> > + info._sifields._sigfault._addr -= 2;
> > + }
>
>
> Am I missing some detail or are both branches of the if-else doing the
> same thing?
Oops, yes: cut-n-paste error; as Philippe says, the not-thumb branch
should be "-= 4".
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
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 9:31 ` Aleksandar Markovic
2020-04-21 9:34 ` Peter Maydell
2 siblings, 1 reply; 19+ messages in thread
From: Aleksandar Markovic @ 2020-04-21 9:31 UTC (permalink / raw)
To: Peter Maydell
Cc: omerg681, qemu-arm, Riku Voipio, QEMU Developers, Laurent Vivier
пон, 20. апр 2020. у 23:25 Peter Maydell <peter.maydell@linaro.org> је
написао/ла:
>
> The kernel has different handling for syscalls with invalid
> numbers that are in the "arm-specific" range 0x9f0000 and up:
> * 0x9f0000..0x9f07ff return -ENOSYS if not implemented
> * other out of range syscalls cause a SIGILL
> (see the kernel's arch/arm/kernel/traps.c:arm_syscall())
>
> Implement this distinction. (Note that our code doesn't look
> quite like the kernel's, because we have removed the
> 0x900000 prefix by this point, whereas the kernel retains
> it in arm_syscall().)
>
Hmm, I suspect other targets could have a similar problem.
I am definitely going to take a look at the mips target, but did
you Peter have a chance to take a more global look whether
this problem is actually widespread?
Regards,
Aleksandar
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> linux-user/arm/cpu_loop.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 025887d6b86..f042108b0be 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -332,10 +332,32 @@ void cpu_loop(CPUARMState *env)
> env->regs[0] = cpu_get_tls(env);
> break;
> default:
> - qemu_log_mask(LOG_UNIMP,
> - "qemu: Unsupported ARM syscall: 0x%x\n",
> - n);
> - env->regs[0] = -TARGET_ENOSYS;
> + 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 */
> + 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);
> + }
> break;
> }
> } else {
> --
> 2.20.1
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
2020-04-21 9:31 ` Aleksandar Markovic
@ 2020-04-21 9:34 ` Peter Maydell
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2020-04-21 9:34 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: omerg681, qemu-arm, Riku Voipio, QEMU Developers, Laurent Vivier
On Tue, 21 Apr 2020 at 10:32, Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
>
> пон, 20. апр 2020. у 23:25 Peter Maydell <peter.maydell@linaro.org> је
> написао/ла:
> >
> > The kernel has different handling for syscalls with invalid
> > numbers that are in the "arm-specific" range 0x9f0000 and up:
> > * 0x9f0000..0x9f07ff return -ENOSYS if not implemented
> > * other out of range syscalls cause a SIGILL
> > (see the kernel's arch/arm/kernel/traps.c:arm_syscall())
> >
> > Implement this distinction. (Note that our code doesn't look
> > quite like the kernel's, because we have removed the
> > 0x900000 prefix by this point, whereas the kernel retains
> > it in arm_syscall().)
> >
>
> Hmm, I suspect other targets could have a similar problem.
>
> I am definitely going to take a look at the mips target, but did
> you Peter have a chance to take a more global look whether
> this problem is actually widespread?
My guess is that this is Arm-specific, because both the OABI-vs-EABI
"do we pass the syscall number in the insn immediate field or
via a register" changeover and also the oddball "arm-specific
handful of syscalls in a distinct range" are Arm hacks, not
something the kernel deals with in generic code.
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers
2020-04-20 21:22 [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
` (2 preceding siblings ...)
2020-04-20 21:22 ` [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly Peter Maydell
@ 2020-04-20 21:22 ` Peter Maydell
2020-04-21 7:57 ` Edgar E. Iglesias
2020-05-12 12:43 ` [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
4 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2020-04-20 21:22 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: omerg681, Riku Voipio, Laurent Vivier
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'
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers
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
0 siblings, 0 replies; 19+ messages in thread
From: Edgar E. Iglesias @ 2020-04-21 7:57 UTC (permalink / raw)
To: Peter Maydell; +Cc: omerg681, qemu-arm, Riku Voipio, qemu-devel, Laurent Vivier
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling
2020-04-20 21:22 [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling Peter Maydell
` (3 preceding siblings ...)
2020-04-20 21:22 ` [PATCH 4/4] linux-user/arm: Fix identification of syscall numbers Peter Maydell
@ 2020-05-12 12:43 ` Peter Maydell
2020-05-18 15:00 ` Peter Maydell
4 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2020-05-12 12:43 UTC (permalink / raw)
To: qemu-arm, QEMU Developers; +Cc: omerg681, Riku Voipio, Laurent Vivier
On Mon, 20 Apr 2020 at 22:22, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchseries fixes issues with the code in linux-user/arm/cpu_loop.c:
> * it incorrectly thinks BKPT is a syscall instruction
> (https://bugs.launchpad.net/qemu/+bug/1873898, reported via irc)
> * a stale line of code means we incorrectly NOP SVC #0xf0002
> * we don't implement the distinction between 0x9f0000..0x9f07ff
> (should return -ENOSYS if not implemented) and higher numbers
> (should cause a SIGILL)
> * we abort() for bad immediate values to SVC (ie not the 0 of EABI
> or the >0x9f0000 of OABI); the kernel delivers a SIGILL for these
> * for Thumb mode, we never use the immediate value from the insn,
> but we always read it anyway
>
> This patchseries fixes all those things. (I started out fixing the
> BKPT bug; everything else is problems I spotted along the way while
> I was reading this bit of code...)
Laurent, do you want me to post a v2 with the -2/-4 thinko fixed
so you can put it via the linux-user tree, or should I just take
this via target-arm.next?
thanks
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] linux-user/arm: Fix BKPT, SVC immediate handling
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
0 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2020-05-18 15:00 UTC (permalink / raw)
To: qemu-arm, QEMU Developers; +Cc: omerg681, Riku Voipio, Laurent Vivier
On Tue, 12 May 2020 at 13:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 20 Apr 2020 at 22:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > This patchseries fixes issues with the code in linux-user/arm/cpu_loop.c:
> > * it incorrectly thinks BKPT is a syscall instruction
> > (https://bugs.launchpad.net/qemu/+bug/1873898, reported via irc)
> > * a stale line of code means we incorrectly NOP SVC #0xf0002
> > * we don't implement the distinction between 0x9f0000..0x9f07ff
> > (should return -ENOSYS if not implemented) and higher numbers
> > (should cause a SIGILL)
> > * we abort() for bad immediate values to SVC (ie not the 0 of EABI
> > or the >0x9f0000 of OABI); the kernel delivers a SIGILL for these
> > * for Thumb mode, we never use the immediate value from the insn,
> > but we always read it anyway
> >
> > This patchseries fixes all those things. (I started out fixing the
> > BKPT bug; everything else is problems I spotted along the way while
> > I was reading this bit of code...)
>
> Laurent, do you want me to post a v2 with the -2/-4 thinko fixed
> so you can put it via the linux-user tree, or should I just take
> this via target-arm.next?
I've applied this series (with the fixup) to target-arm.next; thanks.
-- PMM
^ permalink raw reply [flat|nested] 19+ messages in thread