Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

On 2022-01-27 13:14, Nicholas Piggin wrote:
> Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> Per the ISA, a Trace interrupt is not generated for a system call
>>> [vectored] instruction. Reject uprobes on such instructions as we are
>>> not emulating a system call [vectored] instruction anymore.
>> 
>> This should really be patch 1, otherwise there's a single commit 
>> window
>> where we allow uprobes on sc but don't honour them.
> 
> Yep true. I also messed up Naveen's attribution! Will re-send (or maybe
> Naveen would take over the series).

Yes, let me come up with a better, more complete patch for this.

> 
>> 
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> [np: Switch to pr_info_ratelimited]
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>  arch/powerpc/include/asm/ppc-opcode.h | 1 +
>>>  arch/powerpc/kernel/uprobes.c         | 6 ++++++
>>>  2 files changed, 7 insertions(+)
>>> 
>>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h 
>>> b/arch/powerpc/include/asm/ppc-opcode.h
>>> index 9675303b724e..8bbe16ce5173 100644
>>> --- a/arch/powerpc/include/asm/ppc-opcode.h
>>> +++ b/arch/powerpc/include/asm/ppc-opcode.h
>>> @@ -411,6 +411,7 @@
>>>  #define PPC_RAW_DCBFPS(a, b)		(0x7c0000ac | ___PPC_RA(a) | 
>>> ___PPC_RB(b) | (4 << 21))
>>>  #define PPC_RAW_DCBSTPS(a, b)		(0x7c0000ac | ___PPC_RA(a) | 
>>> ___PPC_RB(b) | (6 << 21))
>>>  #define PPC_RAW_SC()			(0x44000002)
>>> +#define PPC_RAW_SCV()			(0x44000001)
>>>  #define PPC_RAW_SYNC()			(0x7c0004ac)
>>>  #define PPC_RAW_ISYNC()			(0x4c00012c)
>>> 
>>> diff --git a/arch/powerpc/kernel/uprobes.c 
>>> b/arch/powerpc/kernel/uprobes.c
>>> index c6975467d9ff..3779fde804bd 100644
>>> --- a/arch/powerpc/kernel/uprobes.c
>>> +++ b/arch/powerpc/kernel/uprobes.c
>>> @@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe 
>>> *auprobe,
>>>  	if (addr & 0x03)
>>>  		return -EINVAL;
>>> 
>>> +	if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() ||
>>> +	    ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) {
>> 
>> We should probably reject hypercall too?
>> 
>> There's also a lot of reserved fields in `sc`, so doing an exact match
>> like this risks missing instructions that are badly formed but the CPU
>> will happily execute as `sc`.
> 
> Yeah, scv as well has lev != 0 unsupported so should be excluded.
>> 
>> We'd obviously never expect to see those in compiler generated code, 
>> but
>> it'd still be safer to mask. We could probably just reject opcode 17
>> entirely.

Indeed, thanks.

>> 
>> And I guess for a subsequent patch, but we should be rejecting some
>> others here as well shouldn't we? Like rfid etc.
> 
> Traps under discussion I guess. For uprobe, rfid will be just another
> privilege fault. Is that dealt with somehow or do all privileged and
> illegal instructions also need to be excluded from stepping? (I assume
> we must handle that in a general way somehow)

Yes, this is all handled in our interrupt code if we emulate any of 
those
privileged instructions. Otherwise, if a signal is generated, that would
be caught by uprobe_deny_signal().


Thanks,
Naveen