Re: [PATCH] parisc: Fix syscall restarts

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

----- On Dec 20, 2015, at 1:31 PM, John David Anglin dave.anglin@bell.net wrote:

> On 2015-12-20, at 10:49 AM, Helge Deller wrote:
> 
>>>>> /* Check if delay branch uses "copy %rX,%r20" */
>>>>> +	if ((opcode & 0xff00ffff) == 0x08000254) {
>>>>> +		source_reg = (opcode >> 16) & 31;
>>>> 
>>>> Can you explain the reasoning behind the & 31 mask ?
>>>> I'm no parisc expert, but this seems rather odd.
>>>> Do you really mean "% 31" which translates to "& 5" ?
>>>> It would make more sense since there are 32 "gr"
>>>> registers. With & 31, a carefully crafted opcode
>>>> could overflow the gr[32] array, and cause a kernel
>>>> overflow allowing to overwrite kernel memory
>>>> (security issue).
>>> 
>>> Hrm, I got my masks temporarily mixed up (early morning
>>> here). This is why I always use constructs such as:
>>> 
>>> #define GR_REGS_BITS  5
>>> #define NR_GR_REGS    (1U << GR_REGS_BITS)
>>> #define GR_REGS_MASK  (NR_GR_REGS - 1)
>>> 
>>> and then
>>> 
>>> v & GR_REGS_MASK;
>>> 
>>> Which makes everything super-obvious. The & 31 mask
>>> seems therefore technically correct.
>> 
>> Good. I was struggling with your comments as well :-)
>> 
>> 
>>> The paragraph below still holds though:
>>> 
>>>> 
>>>> If it's the case, then it would also be good to
>>>> check that the register selector within the opcode
>>>> is not larger than 31 (e.g. applying to fr or sr
>>>> register), rather than applying the modulo and
>>>> assuming it's a gr and corrupt userspace state.
>> 
>> I'll check that.
> 
> The register field cannot be larger than 31.  A fr or sr
> regster can't be used in a LDO "copy" instruction.

Independently of the instruction set, if an application
pulls the carpet from under the kernel from a concurrent
thread (or possibly ptrace) by forging an invalid
instruction after the system call has started executing,
it would be good to catch it and report it with a printk,
rather than blindly expecting that some bits should always
be 0.

Thanks,

Mathieu

> 
> Dave
> --
> John David Anglin	dave.anglin@bell.net

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com