Re: [PATCH 2/2] arm64: kprobe: disable probe of fault prone ptrauth instruction

From: Amit Kachhap <amit.kachhap@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Kees Cook <keescook@chromium.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Kristina Martsenko <kristina.martsenko@arm.com>,
	Mark Brown <broonie@kernel.org>,
	James Morse <james.morse@arm.com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Will Deacon <will@kernel.org>, Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64: kprobe: disable probe of fault prone ptrauth instruction
Date: Fri, 28 Feb 2020 16:42:10 +0530	[thread overview]
Message-ID: <52db2533-488a-1e27-947c-ec92048f26ae@arm.com> (raw)
In-Reply-To: <20200227164817.GA31259@lakrids.cambridge.arm.com>

Hi,

On 2/27/20 10:18 PM, Mark Rutland wrote:
> Hi Amit,
> 
> On Wed, Feb 19, 2020 at 06:30:40PM +0530, Amit Daniel Kachhap wrote:
>> This patch disables the probing of authenticate ptrauth instruction
>> (AUTIASP) which falls under the hint instructions region. This is done
>> to disallow probe of authenticate instruction in the kernel which may
>> lead to ptrauth faults with the addition of Armv8.6 enhanced ptrauth
>> features.
>>
>> The corresponding append pac ptrauth instruction (PACIASP) is not disabled
>> and they can still be probed.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>>   arch/arm64/include/asm/insn.h          | 13 +++++++------
>>   arch/arm64/kernel/insn.c               |  1 +
>>   arch/arm64/kernel/probes/decode-insn.c |  2 +-
>>   3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index bb313dd..2e01db0 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -40,12 +40,13 @@ enum aarch64_insn_encoding_class {
>>   };
>>   
>>   enum aarch64_insn_hint_op {
>> -	AARCH64_INSN_HINT_NOP	= 0x0 << 5,
>> -	AARCH64_INSN_HINT_YIELD	= 0x1 << 5,
>> -	AARCH64_INSN_HINT_WFE	= 0x2 << 5,
>> -	AARCH64_INSN_HINT_WFI	= 0x3 << 5,
>> -	AARCH64_INSN_HINT_SEV	= 0x4 << 5,
>> -	AARCH64_INSN_HINT_SEVL	= 0x5 << 5,
>> +	AARCH64_INSN_HINT_NOP		= 0x0 << 5,
>> +	AARCH64_INSN_HINT_YIELD		= 0x1 << 5,
>> +	AARCH64_INSN_HINT_WFE		= 0x2 << 5,
>> +	AARCH64_INSN_HINT_WFI		= 0x3 << 5,
>> +	AARCH64_INSN_HINT_SEV		= 0x4 << 5,
>> +	AARCH64_INSN_HINT_SEVL		= 0x5 << 5,
>> +	AARCH64_INSN_HINT_AUTIASP	= (0x3 << 8) | (0x5 << 5),
>>   };
>>   
>>   enum aarch64_insn_imm_type {
>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>> index 4a9e773..87f7c8a 100644
>> --- a/arch/arm64/kernel/insn.c
>> +++ b/arch/arm64/kernel/insn.c
>> @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
>>   	case AARCH64_INSN_HINT_WFI:
>>   	case AARCH64_INSN_HINT_SEV:
>>   	case AARCH64_INSN_HINT_SEVL:
>> +	case AARCH64_INSN_HINT_AUTIASP:
>>   		return false;
>>   	default:
>>   		return true;
> 
> I'm afraid that the existing code here is simply wrong, and this is
> adding to the mess.
> 
> We have no idea what HINT space instructions will be in the future, so
> the only sensible implementations of aarch64_insn_is_nop() are something
> like:
> 
> bool __kprobes aarch64_insn_is_nop(u32 insn)
> {
> 	return insn == aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
> }
> 
> ... and if we want to check for other HINT instructions, they should be
> checked explicitly.
> 
> Can you please change aarch64_insn_is_nop() as above?

Agree that current implementation is unclear.
I will implement as you suggested.

Cheers,
Amit

> 
> Generally the logic in aarch64_insn_is_steppable() needs to be reworked
> to a whitelist, but at least chagning aarch64_insn_is_nop() this way is
> closer to where we want to be.
> 
> Thanks,
> Mark.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel