linux-kernel - Re: [PATCH v2 2/2] arm64/module: Use text-poke API for late relocations.

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250509161508.GB5984@willie-the-truck>
Date: Fri, 9 May 2025 17:15:08 +0100
From: Will Deacon <will@...nel.org>
To: Dylan Hatch <dylanbhatch@...gle.com>
Cc: Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	Eduard Zingerman <eddyz87@...il.com>,
	Yonghong Song <yonghong.song@...ux.dev>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	Puranjay Mohan <puranjay@...nel.org>,
	Xu Kuohai <xukuohai@...weicloud.com>,
	Philippe Mathieu-Daudé <philmd@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	"Mike Rapoport (Microsoft)" <rppt@...nel.org>,
	Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Luis Chamberlain <mcgrof@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Song Liu <song@...nel.org>, Ard Biesheuvel <ardb@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Roman Gushchin <roman.gushchin@...ux.dev>
Subject: Re: [PATCH v2 2/2] arm64/module: Use text-poke API for late
 relocations.

Hi Dylan,

On Sat, Apr 12, 2025 at 01:09:39AM +0000, Dylan Hatch wrote:
> To enable late module patching, livepatch modules need to be able to
> apply some of their relocations well after being loaded. In this
> scenario, use the text-poking API to allow this, even with
> STRICT_MODULE_RWX.
> 
> This patch is largely based off commit 88fc078a7a8f6 ("x86/module: Use
> text_poke() for late relocations").
> 
> Signed-off-by: Dylan Hatch <dylanbhatch@...gle.com>
> ---
>  arch/arm64/kernel/module.c | 129 ++++++++++++++++++++++++-------------
>  1 file changed, 83 insertions(+), 46 deletions(-)

On its own, this isn't gaining us an awful lot upstream as we don't have
livepatch support (arm64 does not select HAVE_LIVEPATCH), however I'm
not against incremental changes towards enabling that. Are you planning
to work on follow-up changes for the rest of the support?

> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 06bb680bfe975..0703502d9dc37 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -18,11 +18,13 @@
>  #include <linux/moduleloader.h>
>  #include <linux/random.h>
>  #include <linux/scs.h>
> +#include <linux/memory.h>
>  
>  #include <asm/alternative.h>
>  #include <asm/insn.h>
>  #include <asm/scs.h>
>  #include <asm/sections.h>
> +#include <asm/text-patching.h>
>  
>  enum aarch64_reloc_op {
>  	RELOC_OP_NONE,
> @@ -48,7 +50,8 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
>  	return 0;
>  }
>  
> -static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> +static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
> +		      void *(*write)(void *dest, const void *src, size_t len))
>  {

I think it's a bit grotty indirecting the write when in reality we either
need a straight-forward assignment (like we have today) or a call to
an instruction-patching helper.

In particular, when you get to functions such as:

>  static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
> -			   int lsb, enum aarch64_insn_movw_imm_type imm_type)
> +			   int lsb, enum aarch64_insn_movw_imm_type imm_type,
> +			   void *(*write)(void *dest, const void *src, size_t len))
>  {
>  	u64 imm;
>  	s64 sval;
>  	u32 insn = le32_to_cpu(*place);
> +	__le32 le_insn;
>  
>  	sval = do_reloc(op, place, val);
>  	imm = sval >> lsb;
> @@ -145,7 +150,8 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
>  
>  	/* Update the instruction with the new encoding. */
>  	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
> -	*place = cpu_to_le32(insn);
> +	le_insn = cpu_to_le32(insn);
> +	write(place, &le_insn, sizeof(le_insn));

we're forced into passing &le_insn because we need the same function
prototype as memcpy().

Instead, how about we pass the 'struct module *mod' pointer down from
apply_relocate_add()? That's already done for reloc_insn_adrp() and it
would mean that the above could be written as:


static int reloc_insn_movw(struct module *mod, enum aarch64_reloc_op op,
			   __le32 *place, u64 val, int lsb,
			   enum aarch64_insn_movw_imm_type imm_type)
{
	...

	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
	insn = cpu_to_le32(insn);

	if (mod->state != MODULE_STATE_UNFORMED)
		aarch64_insn_set(place, insn, sizeof(insn));
	else
		*place = insn;


meaning we can also drop the first patch, because I don't think we need
a text_poke() helper.

> +int apply_relocate_add(Elf64_Shdr *sechdrs,
> +		       const char *strtab,
> +		       unsigned int symindex,
> +		       unsigned int relsec,
> +		       struct module *me)
> +{
> +	int ret;
> +	bool early = me->state == MODULE_STATE_UNFORMED;
> +	void *(*write)(void *, const void *, size_t) = memcpy;
> +
> +	if (!early) {
> +		write = text_poke;
> +		mutex_lock(&text_mutex);
> +	}
> +
> +	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> +				   write);
> +
> +	if (!early)
> +		mutex_unlock(&text_mutex);

Why is the responsibility of the arch code to take the 'text_mutex' here?
The core code should be able to do that when the module state is !=
MODULE_STATE_UNFORMED.

Cheers,

Will