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

From: Mark Rutland <mark.rutland@arm.com>
To: Amit Daniel Kachhap <amit.kachhap@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: Thu, 27 Feb 2020 16:48:17 +0000	[thread overview]
Message-ID: <20200227164817.GA31259@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <1582117240-15330-3-git-send-email-amit.kachhap@arm.com>

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?

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