[PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly

Peter Maydell posted 4 patches 5 years, 9 months ago
Maintainers: Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>
[PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Peter Maydell 5 years, 9 months ago
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


Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Edgar E. Iglesias 5 years, 9 months ago
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
> 
> 

Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
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
>>
>>
> 

Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Peter Maydell 5 years, 9 months ago
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

Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Aleksandar Markovic 5 years, 9 months ago
пон, 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
>
>

Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Peter Maydell 5 years, 9 months ago
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

Re: [PATCH 3/4] linux-user/arm: Handle invalid arm-specific syscalls correctly
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
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>