----- On Dec 21, 2015, at 4:19 AM, Helge Deller deller@xxxxxx wrote:
> This is version 2 of the patch:
>
> On parisc syscalls which are interrupted by signals sometimes failed to
> restart and instead returned -ENOSYS which in the worst case lead to
> userspace crashes.
> A similiar problem existed on MIPS and was fixed by commit e967ef02
> ("MIPS: Fix restart of indirect syscalls").
>
> On parisc the current syscall restart code assumes that all syscall
> callers load the syscall number in the delay slot of the ble
> instruction. That's how it is e.g. done in the unistd.h header file:
> ble 0x100(%sr2, %r0)
> ldi #syscall_nr, %r20
> Because of that assumption the current code never restored %r20 before
> returning to userspace.
>
> This assumption is at least not true for code which uses the glibc
> syscall() function, which instead uses this syntax:
> ble 0x100(%sr2, %r0)
> copy regX, %r20
> where regX depend on how the compiler optimizes the code and register
> usage.
>
> This patch fixes this problem by adding code to analyze how the syscall
> number is loaded in the delay branch and - if needed - copy the syscall
> number to regX prior returning to userspace for the syscall restart.
>
> Signed-off-by: Helge Deller <deller@xxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
>
> diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
> index dc1ea79..2264f68 100644
> --- a/arch/parisc/kernel/signal.c
> +++ b/arch/parisc/kernel/signal.c
> @@ -435,6 +435,55 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs,
> int in_syscall)
> regs->gr[28]);
> }
>
> +/*
> + * Check how the syscall number gets loaded into %r20 within
> + * the delay branch in userspace and adjust as needed.
> + */
> +
> +static void check_syscallno_in_delay_branch(struct pt_regs *regs)
> +{
> + u32 opcode, source_reg;
> + u32 __user *uaddr;
> + int err;
> +
> + /* Usually we don't have to restore %r20 (the system call number)
> + * because it gets loaded in the delay slot of the branch external
> + * instruction via the ldi instruction.
> + * In some cases a register-to-register copy instruction might have
> + * been used instead, in which case we need to copy the syscall
> + * number into the source register before returning to userspace.
> + */
> +
> + /* A syscall is just a branch, so all we have to do is fiddle the
> + * return pointer so that the ble instruction gets executed again.
> + */
> + regs->gr[31] -= 8; /* delayed branching */
> +
> + /* Get assembler opcode of code in delay branch */
> + uaddr = (unsigned int *) ((regs->gr[31] & ~3) + 4);
Is it valid to have unaligned instructions ? Does the architecture
allow it, or it's a fumble and we should pr_warn ?
> + err = get_user(opcode, uaddr);
> + if (err)
Should we add a pr_warn here ?
> + return;
> +
> + /* Check if delay branch uses "ldi int,%r20" */
> + if ((opcode & 0xffff0000) == 0x34140000)
> + return; /* everything ok, just return */
> +
> + /* Check if delay branch uses "nop" */
> + if (opcode == INSN_NOP)
> + return;
When we find a NOP in the delay slot, how can we be sure %r20
still holds the syscall value when we re-play the branch
instruction ? Can it be overwritten during the syscall,
either from start of syscall to here, or from here to
return to userspace ?
> +
> + /* Check if delay branch uses "copy %rX,%r20" */
> + if ((opcode & 0xffe0ffff) == 0x08000254) {
> + source_reg = (opcode >> 16) & 31;
> + regs->gr[source_reg] = regs->gr[20];
Similar question here, how can we be sure regs->gr[20]
still has the system call number at this point (not
overwritten from start of syscall to here) ?
Thanks,
Mathieu
> + return;
> + }
> +
> + pr_warn("syscall restart: %s (pid %d): unexpected opcode 0x%08x\n",
> + current->comm, task_pid_nr(current), opcode);
> +}
> +
> static inline void
> syscall_restart(struct pt_regs *regs, struct k_sigaction *ka)
> {
> @@ -457,10 +506,7 @@ syscall_restart(struct pt_regs *regs, struct k_sigaction
> *ka)
> }
> /* fallthrough */
> case -ERESTARTNOINTR:
> - /* A syscall is just a branch, so all
> - * we have to do is fiddle the return pointer.
> - */
> - regs->gr[31] -= 8; /* delayed branching */
> + check_syscallno_in_delay_branch(regs);
> break;
> }
> }
> @@ -510,15 +556,9 @@ insert_restart_trampoline(struct pt_regs *regs)
> }
> case -ERESTARTNOHAND:
> case -ERESTARTSYS:
> - case -ERESTARTNOINTR: {
> - /* Hooray for delayed branching. We don't
> - * have to restore %r20 (the system call
> - * number) because it gets loaded in the delay
> - * slot of the branch external instruction.
> - */
> - regs->gr[31] -= 8;
> + case -ERESTARTNOINTR:
> + check_syscallno_in_delay_branch(regs);
> return;
> - }
> default:
> break;
> }
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
