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

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.

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(+)

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.

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

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.

cheers


> +		pr_info_ratelimited("Rejecting uprobe on system call instruction\n");
> +		return -EINVAL;
> +	}
> +
>  	if (cpu_has_feature(CPU_FTR_ARCH_31) &&
>  	    ppc_inst_prefixed(ppc_inst_read(auprobe->insn)) &&
>  	    (addr & 0x3f) == 60) {
> -- 
> 2.23.0

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

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