[PATCH v3 0/3] arm64: Make NOP handling a whitelist

[PATCH v3 0/3] arm64: Make NOP handling a whitelist

[Mark Brown]

Currently we default to assuming any unrecognized instruction in the
hint space can be safely handled as a NOP.  This is not robust and any
code that really wants a NOP should be using the explicitly defined NOP
so let's instead invert this and whitelist those instructions which it
is safe to handle as NOPs.

Patch 2 adds defines for the HINTs for BTI landing pads which will be
used by the in-kernel BTI series to generate landing pads in JITed BPF
code so it'd be good if this could be applied on or merged into the BTI
branch.

v3:
 - Correct values for more of the HINTs.
 - Add constants for additional HINTs 
 - Handle XPACLRI as a NOP along with all the other PAC HINTs.
 - Update commit messages for greater clarity.
 - Tweak the comment about what a NOP.
v2:
 - Fix values for BTI HINTs.
 - Rebase on v5.7-rc3+for-next/bti-user

Mark Brown (3):
  arm64: insn: Don't assume unrecognized HINTs are NOPs
  arm64: insn: Add constants for new HINT instruction decode
  arm64: insn: Report PAC and BTI instructions as NOPs

 arch/arm64/include/asm/insn.h | 28 ++++++++++++++++++++++++--
 arch/arm64/kernel/insn.c      | 37 ++++++++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 11 deletions(-)

-- 
2.20.1


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

[PATCH v3 1/3] arm64: insn: Don't assume unrecognized HINTs are NOPs

[Mark Brown]

Currently the kernel assumes that any HINT which it does not explicitly
recognise is a NOP.  This is not robust as new instructions may be added
which need special handling, including recent extensions like PAC, and
in any case software should only be using explicit NOP instructions for
deliberate NOPs.

This has the effect of rendering PAC and BTI instructions unprobeable
which means that probes can't be inserted on the first instruction of
functions built with those features.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/insn.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 4a9e773a177f..595c0fddeeb2 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -51,21 +51,17 @@ enum aarch64_insn_encoding_class __kprobes aarch64_get_insn_class(u32 insn)
 	return aarch64_insn_encoding_class[(insn >> 25) & 0xf];
 }
 
-/* NOP is an alias of HINT */
+/* NOP is a specific HINT */
 bool __kprobes aarch64_insn_is_nop(u32 insn)
 {
 	if (!aarch64_insn_is_hint(insn))
 		return false;
 
 	switch (insn & 0xFE0) {
-	case AARCH64_INSN_HINT_YIELD:
-	case AARCH64_INSN_HINT_WFE:
-	case AARCH64_INSN_HINT_WFI:
-	case AARCH64_INSN_HINT_SEV:
-	case AARCH64_INSN_HINT_SEVL:
-		return false;
-	default:
+	case AARCH64_INSN_HINT_NOP:
 		return true;
+	default:
+		return false;
 	}
 }
 
-- 
2.20.1


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

[PATCH v3 2/3] arm64: insn: Add constants for new HINT instruction decode

[Mark Brown]

Add constants for decoding newer instructions defined in the HINT space.
Since we are now decoding both the op2 and CRm fields rename the enum as
well; this is compatible with what the existing users are doing.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/insn.h | 28 ++++++++++++++++++++++++++--
 arch/arm64/kernel/insn.c      |  2 +-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index bb313dde58a4..575675145fe2 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -39,13 +39,37 @@ enum aarch64_insn_encoding_class {
 					 * system instructions */
 };
 
-enum aarch64_insn_hint_op {
+enum aarch64_insn_hint_cr_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_XPACLRI    = 0x07 << 5,
+	AARCH64_INSN_HINT_PACIA_1716 = 0x08 << 5,
+	AARCH64_INSN_HINT_PACIB_1716 = 0x0A << 5,
+	AARCH64_INSN_HINT_AUTIA_1716 = 0x0C << 5,
+	AARCH64_INSN_HINT_AUTIB_1716 = 0x0E << 5,
+	AARCH64_INSN_HINT_PACIAZ     = 0x18 << 5,
+	AARCH64_INSN_HINT_PACIASP    = 0x19 << 5,
+	AARCH64_INSN_HINT_PACIBZ     = 0x1A << 5,
+	AARCH64_INSN_HINT_PACIBSP    = 0x1B << 5,
+	AARCH64_INSN_HINT_AUTIAZ     = 0x1C << 5,
+	AARCH64_INSN_HINT_AUTIASP    = 0x1D << 5,
+	AARCH64_INSN_HINT_AUTIBZ     = 0x1E << 5,
+	AARCH64_INSN_HINT_AUTIBSP    = 0x1F << 5,
+
+	AARCH64_INSN_HINT_ESB  = 0x10 << 5,
+	AARCH64_INSN_HINT_PSB  = 0x11 << 5,
+	AARCH64_INSN_HINT_TSB  = 0x12 << 5,
+	AARCH64_INSN_HINT_CSDB = 0x14 << 5,
+
+	AARCH64_INSN_HINT_BTI   = 0x20 << 5,
+	AARCH64_INSN_HINT_BTIC  = 0x22 << 5,
+	AARCH64_INSN_HINT_BTIJ  = 0x24 << 5,
+	AARCH64_INSN_HINT_BTIJC = 0x26 << 5,
 };
 
 enum aarch64_insn_imm_type {
@@ -370,7 +394,7 @@ u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
 				     enum aarch64_insn_branch_type type);
 u32 aarch64_insn_gen_cond_branch_imm(unsigned long pc, unsigned long addr,
 				     enum aarch64_insn_condition cond);
-u32 aarch64_insn_gen_hint(enum aarch64_insn_hint_op op);
+u32 aarch64_insn_gen_hint(enum aarch64_insn_hint_cr_op op);
 u32 aarch64_insn_gen_nop(void);
 u32 aarch64_insn_gen_branch_reg(enum aarch64_insn_register reg,
 				enum aarch64_insn_branch_type type);
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 595c0fddeeb2..d458604a96a3 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -570,7 +570,7 @@ u32 aarch64_insn_gen_cond_branch_imm(unsigned long pc, unsigned long addr,
 					     offset >> 2);
 }
 
-u32 __kprobes aarch64_insn_gen_hint(enum aarch64_insn_hint_op op)
+u32 __kprobes aarch64_insn_gen_hint(enum aarch64_insn_hint_cr_op op)
 {
 	return aarch64_insn_get_hint_value() | op;
 }
-- 
2.20.1


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

[PATCH v3 3/3] arm64: insn: Report PAC and BTI instructions as NOPs

[Mark Brown]

In order to allow probing of PAC and BTI instructions without more
specialized support recognize them as NOPs.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/insn.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index d458604a96a3..aa525975bf94 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -57,7 +57,30 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
 	if (!aarch64_insn_is_hint(insn))
 		return false;
 
+	/*
+	 * The PAC and BTI instructons are not strictly NOPs but until
+	 * better support is added we can treat them as such and they
+	 * are commonly placed in locations where it is useful to be
+	 * able to probe.
+	 */
 	switch (insn & 0xFE0) {
+	case AARCH64_INSN_HINT_XPACLRI:
+	case AARCH64_INSN_HINT_PACIA_1716:
+	case AARCH64_INSN_HINT_PACIB_1716:
+	case AARCH64_INSN_HINT_AUTIA_1716:
+	case AARCH64_INSN_HINT_AUTIB_1716:
+	case AARCH64_INSN_HINT_PACIAZ:
+	case AARCH64_INSN_HINT_PACIASP:
+	case AARCH64_INSN_HINT_PACIBZ:
+	case AARCH64_INSN_HINT_PACIBSP:
+	case AARCH64_INSN_HINT_AUTIAZ:
+	case AARCH64_INSN_HINT_AUTIASP:
+	case AARCH64_INSN_HINT_AUTIBZ:
+	case AARCH64_INSN_HINT_AUTIBSP:
+	case AARCH64_INSN_HINT_BTI:
+	case AARCH64_INSN_HINT_BTIC:
+	case AARCH64_INSN_HINT_BTIJ:
+	case AARCH64_INSN_HINT_BTIJC:
 	case AARCH64_INSN_HINT_NOP:
 		return true;
 	default:
-- 
2.20.1


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

Re: [PATCH v3 3/3] arm64: insn: Report PAC and BTI instructions as NOPs

[Mark Rutland]

On Fri, May 01, 2020 at 01:37:09PM +0100, Mark Brown wrote:
> In order to allow probing of PAC and BTI instructions without more
> specialized support recognize them as NOPs.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/kernel/insn.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index d458604a96a3..aa525975bf94 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -57,7 +57,30 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
>  	if (!aarch64_insn_is_hint(insn))
>  		return false;
>  
> +	/*
> +	 * The PAC and BTI instructons are not strictly NOPs but until
> +	 * better support is added we can treat them as such and they
> +	 * are commonly placed in locations where it is useful to be
> +	 * able to probe.
> +	 */
>  	switch (insn & 0xFE0) {
> +	case AARCH64_INSN_HINT_XPACLRI:
> +	case AARCH64_INSN_HINT_PACIA_1716:
> +	case AARCH64_INSN_HINT_PACIB_1716:
> +	case AARCH64_INSN_HINT_AUTIA_1716:
> +	case AARCH64_INSN_HINT_AUTIB_1716:
> +	case AARCH64_INSN_HINT_PACIAZ:
> +	case AARCH64_INSN_HINT_PACIASP:
> +	case AARCH64_INSN_HINT_PACIBZ:
> +	case AARCH64_INSN_HINT_PACIBSP:
> +	case AARCH64_INSN_HINT_AUTIAZ:
> +	case AARCH64_INSN_HINT_AUTIASP:
> +	case AARCH64_INSN_HINT_AUTIBZ:
> +	case AARCH64_INSN_HINT_AUTIBSP:
> +	case AARCH64_INSN_HINT_BTI:
> +	case AARCH64_INSN_HINT_BTIC:
> +	case AARCH64_INSN_HINT_BTIJ:
> +	case AARCH64_INSN_HINT_BTIJC:
>  	case AARCH64_INSN_HINT_NOP:
>  		return true;
>  	default:

I appreciate the desire to not change this code too much, but could we
please rename this to aarch64_insn_is_steppable_hint() to avoid the
misleading name?

This is only ever used in the stepping code today, so the impact is just
the prototype/declaration/callside, but it avoids confusion in future
and renders the comment redundant. If we really need to identify true
nops in future we can add a new aarch64_insn_is_nop() implementation
just for that.

Thanks,
Mark.

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

Re: [PATCH v3 3/3] arm64: insn: Report PAC and BTI instructions as NOPs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 720 bytes --]

On Fri, May 01, 2020 at 01:57:34PM +0100, Mark Rutland wrote:

> I appreciate the desire to not change this code too much, but could we
> please rename this to aarch64_insn_is_steppable_hint() to avoid the
> misleading name?

That's definitely clearer.  I've got this change locally but looking at
the new function name I can see that having done the rename people might
want to go through and also make sure that the list of HINTs we mark as
steppable is up to date which is probably a good idea but would hold
things up further, some of the barriers look like they might need a bit
of thought.

Will, what would you prefer - should I send out a version with the
just rename, do that incrementally or some other thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 3/3] arm64: insn: Report PAC and BTI instructions as NOPs

[Will Deacon]

On Fri, May 01, 2020 at 05:12:10PM +0100, Mark Brown wrote:
> On Fri, May 01, 2020 at 01:57:34PM +0100, Mark Rutland wrote:
> 
> > I appreciate the desire to not change this code too much, but could we
> > please rename this to aarch64_insn_is_steppable_hint() to avoid the
> > misleading name?
> 
> That's definitely clearer.  I've got this change locally but looking at
> the new function name I can see that having done the rename people might
> want to go through and also make sure that the list of HINTs we mark as
> steppable is up to date which is probably a good idea but would hold
> things up further, some of the barriers look like they might need a bit
> of thought.
> 
> Will, what would you prefer - should I send out a version with the
> just rename, do that incrementally or some other thing?

Please send a version with the rename and then people can add extra
instructions to the whitelist as and when they are needed.

Will

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