Re: [PATCH] parisc: Fix syscall restarts (v2)

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Dec 21, 2015, at 3:54 PM, Helge Deller deller@gmx.de wrote:

> On 21.12.2015 21:27, Mathieu Desnoyers wrote:
>> ----- On Dec 21, 2015, at 4:19 AM, Helge Deller deller@gmx.de 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@gmx.de>
>>> Cc: stable@vger.kernel.org
>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>>
>>> 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