linux-kernel - Re: [RFC PATCH 14/16] arm64/insn: always inline aarch64_insn_encode_ldst

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <86h5vtwl7u.wl-maz@kernel.org>
Date: Mon, 20 Oct 2025 18:12:53 +0100
From: Marc Zyngier <maz@...nel.org>
To: Ada Couprie Diaz <ada.coupriediaz@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Ard Biesheuvel <ardb@...nel.org>,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Vincenzo Frascino <vincenzo.frascino@....com>,
	linux-kernel@...r.kernel.org,
	kvmarm@...ts.linux.dev,
	kasan-dev@...glegroups.com,
	Mark Rutland <mark.rutland@....com>
Subject: Re: [RFC PATCH 14/16] arm64/insn: always inline aarch64_insn_encode_ldst_size()

On Tue, 23 Sep 2025 18:49:01 +0100,
Ada Couprie Diaz <ada.coupriediaz@....com> wrote:
> 
> The type and instruction checks cannot be made at compile time,
> as they are dynamically created. However, we can remove the error print
> as it should never appear in normal operation and will still lead to
> a fault BRK.
> 
> This makes `aarch64_insn_encode_ldst_size()` safe for inlining
> and usage from patching callbacks.
> 
> This is a change of visiblity, as previously the function was private to
> lib/insn.c.
> However, in order to inline more `aarch64_insn_` functions and make
> patching callbacks safe, it needs to be accessible by those functions.
> As it is more accessible than before, add a check so that only loads
> or stores can be affected by the size encoding.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@....com>
> ---
>  arch/arm64/include/asm/insn.h | 24 ++++++++++++++++++++++++
>  arch/arm64/lib/insn.c         | 19 +------------------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 44435eede1f3..46d4d452e2e2 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -717,6 +717,30 @@ static __always_inline u32 aarch64_insn_encode_immediate(
>  
>  	return insn;
>  }
> +
> +extern const u32 aarch64_insn_ldst_size[];
> +static __always_inline u32 aarch64_insn_encode_ldst_size(
> +					 enum aarch64_insn_size_type type,
> +					 u32 insn)
> +{
> +	u32 size;
> +
> +	if (type < AARCH64_INSN_SIZE_8 || type > AARCH64_INSN_SIZE_64) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	/* Don't corrput the top bits of other instructions which aren't a size. */
> +	if (!aarch64_insn_is_ldst(insn)) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	size = aarch64_insn_ldst_size[type];

Given that we have this:

	enum aarch64_insn_size_type {
		AARCH64_INSN_SIZE_8,
		AARCH64_INSN_SIZE_16,
		AARCH64_INSN_SIZE_32,
		AARCH64_INSN_SIZE_64,
	};

[...]

> +	insn &= ~GENMASK(31, 30);
> +	insn |= size << 30;
> +
> +	return insn;
> +}
> +
>  static __always_inline u32 aarch64_insn_encode_register(
>  				 enum aarch64_insn_register_type type,
>  				 u32 insn,
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 71df4d72ac81..63564d236235 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -42,30 +42,13 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
>  	return (insn >> shift) & mask;
>  }
>  
> -static const u32 aarch64_insn_ldst_size[] = {
> +const u32 aarch64_insn_ldst_size[] = {
>  	[AARCH64_INSN_SIZE_8] = 0,
>  	[AARCH64_INSN_SIZE_16] = 1,
>  	[AARCH64_INSN_SIZE_32] = 2,
>  	[AARCH64_INSN_SIZE_64] = 3,
>  };

[...] this array is completely superfluous, and

	size = aarch64_insn_ldst_size[type];

could be replaced by

	size = type;

But that's only a very minor improvement. On the plus side, your
approach definitely makes it impossible to add a patching callback
using aarch64_insn_encode_ldst_size() in a module!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.