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

[Nicholas Piggin <npiggin@gmail.com>]

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).

> 
>> 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.
> 
> 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)


Thanks,
Nick