----- On Dec 21, 2015, at 3:54 PM, Helge Deller deller@xxxxxx wrote:
> On 21.12.2015 21:27, Mathieu Desnoyers wrote:
>> ----- 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 ?
>
> How can it be unaligned? It's about u32...
That would be an instruction that is volountarily offset
from 1, 2, 3 bytes from 4-bytes multiples by the application.
The only situation where I have seen this is in cases where
applications are trying to play games with the debugger or
disassembler and hide what they are doing: they can offset
the start of a function like this, and therefore all the
instructions within that function.
> And, no, unaligned instructions are not allowed (I think that at least).
Might be interesting to try it out though. I'm not saying it's
a valid use-case for an application, but it would be at least good
to known whether this is an input we can expect.
>
>>> + err = get_user(opcode, uaddr);
>>> + if (err)
>>
>> Should we add a pr_warn here ?
>
> No. There is no gain to have a warning here.
Allright.
>
>>> + 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 ?
>
> I looked at the code and even tested it (with your testcase actually).
>
>> Can it be overwritten during the syscall,
>> either from start of syscall to here, or from here to
>> return to userspace ?
>
> No.
>
>>> +
>>> + /* 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) ?
>
> Those registers are saved at entry of syscall and
> restored at exit (with exception of a few registers
> e.g. like r28 which is the return value of the syscall).
Can r28 be used as a source_reg ? If so, what happens then ?
Thanks,
Mathieu
>
> Helge
--
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
