| 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
| ||
|
Message-ID:
<TY4PR01MB1377739F1CC08549A619C8635D7BA2@TY4PR01MB13777.jpnprd01.prod.outlook.com>
Date: Wed, 23 Apr 2025 06:04:44 +0000
From: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
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>, 'Will
Deacon' <will@...nel.org>, "'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-arm-kernel@...ts.infradead.org>, "'linux-kernel@...r.kernel.org'"
<linux-kernel@...r.kernel.org>, 'Roman Gushchin' <roman.gushchin@...ux.dev>,
"Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
Subject: RE: [PATCH v2 2/2] arm64/module: Use text-poke API for late
relocations.
Hi Dylan,
> 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(-)
>
> 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))
> {
> s64 sval = do_reloc(op, place, val);
>
> @@ -66,7 +69,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
>
> switch (len) {
> case 16:
> - *(s16 *)place = sval;
> + write(place, &sval, sizeof(s16));
> switch (op) {
> case RELOC_OP_ABS:
> if (sval < 0 || sval > U16_MAX)
> @@ -82,7 +85,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> }
> break;
> case 32:
> - *(s32 *)place = sval;
> + write(place, &sval, sizeof(s32));
> switch (op) {
> case RELOC_OP_ABS:
> if (sval < 0 || sval > U32_MAX)
> @@ -98,7 +101,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> }
> break;
> case 64:
> - *(s64 *)place = sval;
> + write(place, &sval, sizeof(s64));
> break;
> default:
> pr_err("Invalid length (%d) for data relocation\n", len);
> @@ -113,11 +116,13 @@ enum aarch64_insn_movw_imm_type {
> };
>
> 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));
>
> if (imm > U16_MAX)
> return -ERANGE;
> @@ -154,11 +160,13 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
> }
>
> static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
> - int lsb, int len, enum aarch64_insn_imm_type imm_type)
> + int lsb, int len, enum aarch64_insn_imm_type imm_type,
> + void *(*write)(void *dest, const void *src, size_t len))
> {
> u64 imm, imm_mask;
> s64 sval;
> u32 insn = le32_to_cpu(*place);
> + __le32 le_insn;
>
> /* Calculate the relocation value. */
> sval = do_reloc(op, place, val);
> @@ -170,7 +178,8 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
>
> /* Update the instruction's immediate field. */
> insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
> - *place = cpu_to_le32(insn);
> + le_insn = cpu_to_le32(insn);
> + write(place, &le_insn, sizeof(le_insn));
>
> /*
> * Extract the upper value bits (including the sign bit) and
> @@ -189,17 +198,19 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
> }
>
> static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> - __le32 *place, u64 val)
> + __le32 *place, u64 val,
> + void *(*write)(void *dest, const void *src, size_t len))
> {
> u32 insn;
> + __le32 le_insn;
>
> if (!is_forbidden_offset_for_adrp(place))
> return reloc_insn_imm(RELOC_OP_PAGE, place, val, 12, 21,
> - AARCH64_INSN_IMM_ADR);
> + AARCH64_INSN_IMM_ADR, write);
>
> /* patch ADRP to ADR if it is in range */
> if (!reloc_insn_imm(RELOC_OP_PREL, place, val & ~0xfff, 0, 21,
> - AARCH64_INSN_IMM_ADR)) {
> + AARCH64_INSN_IMM_ADR, write)) {
> insn = le32_to_cpu(*place);
> insn &= ~BIT(31);
> } else {
> @@ -211,15 +222,17 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> AARCH64_INSN_BRANCH_NOLINK);
> }
>
> - *place = cpu_to_le32(insn);
> + le_insn = cpu_to_le32(insn);
> + write(place, &le_insn, sizeof(le_insn));
> return 0;
> }
>
> -int apply_relocate_add(Elf64_Shdr *sechdrs,
> +static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> const char *strtab,
> unsigned int symindex,
> unsigned int relsec,
> - struct module *me)
> + struct module *me,
> + void *(*write)(void *dest, const void *src, size_t len))
> {
> unsigned int i;
> int ovf;
> @@ -255,23 +268,23 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> /* Data relocations. */
> case R_AARCH64_ABS64:
> overflow_check = false;
> - ovf = reloc_data(RELOC_OP_ABS, loc, val, 64);
> + ovf = reloc_data(RELOC_OP_ABS, loc, val, 64, write);
> break;
> case R_AARCH64_ABS32:
> - ovf = reloc_data(RELOC_OP_ABS, loc, val, 32);
> + ovf = reloc_data(RELOC_OP_ABS, loc, val, 32, write);
> break;
> case R_AARCH64_ABS16:
> - ovf = reloc_data(RELOC_OP_ABS, loc, val, 16);
> + ovf = reloc_data(RELOC_OP_ABS, loc, val, 16, write);
> break;
> case R_AARCH64_PREL64:
> overflow_check = false;
> - ovf = reloc_data(RELOC_OP_PREL, loc, val, 64);
> + ovf = reloc_data(RELOC_OP_PREL, loc, val, 64, write);
> break;
> case R_AARCH64_PREL32:
> - ovf = reloc_data(RELOC_OP_PREL, loc, val, 32);
> + ovf = reloc_data(RELOC_OP_PREL, loc, val, 32, write);
> break;
> case R_AARCH64_PREL16:
> - ovf = reloc_data(RELOC_OP_PREL, loc, val, 16);
> + ovf = reloc_data(RELOC_OP_PREL, loc, val, 16, write);
> break;
>
> /* MOVW instruction relocations. */
> @@ -280,88 +293,88 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> fallthrough;
> case R_AARCH64_MOVW_UABS_G0:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 0,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_UABS_G1_NC:
> overflow_check = false;
> fallthrough;
> case R_AARCH64_MOVW_UABS_G1:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 16,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_UABS_G2_NC:
> overflow_check = false;
> fallthrough;
> case R_AARCH64_MOVW_UABS_G2:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 32,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_UABS_G3:
> /* We're using the top bits so we can't overflow. */
> overflow_check = false;
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 48,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_SABS_G0:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 0,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_SABS_G1:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 16,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_SABS_G2:
> ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 32,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G0_NC:
> overflow_check = false;
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 0,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G0:
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 0,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G1_NC:
> overflow_check = false;
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 16,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G1:
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 16,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G2_NC:
> overflow_check = false;
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 32,
> - AARCH64_INSN_IMM_MOVKZ);
> + AARCH64_INSN_IMM_MOVKZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G2:
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 32,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
> case R_AARCH64_MOVW_PREL_G3:
> /* We're using the top bits so we can't overflow. */
> overflow_check = false;
> ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 48,
> - AARCH64_INSN_IMM_MOVNZ);
> + AARCH64_INSN_IMM_MOVNZ, write);
> break;
>
> /* Immediate instruction relocations. */
> case R_AARCH64_LD_PREL_LO19:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 19,
> - AARCH64_INSN_IMM_19);
> + AARCH64_INSN_IMM_19, write);
> break;
> case R_AARCH64_ADR_PREL_LO21:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 0, 21,
> - AARCH64_INSN_IMM_ADR);
> + AARCH64_INSN_IMM_ADR, write);
> break;
> case R_AARCH64_ADR_PREL_PG_HI21_NC:
> overflow_check = false;
> fallthrough;
> case R_AARCH64_ADR_PREL_PG_HI21:
> - ovf = reloc_insn_adrp(me, sechdrs, loc, val);
> + ovf = reloc_insn_adrp(me, sechdrs, loc, val, write);
> if (ovf && ovf != -ERANGE)
> return ovf;
> break;
> @@ -369,46 +382,46 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> case R_AARCH64_LDST8_ABS_LO12_NC:
> overflow_check = false;
> ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 0, 12,
> - AARCH64_INSN_IMM_12);
> + AARCH64_INSN_IMM_12, write);
> break;
> case R_AARCH64_LDST16_ABS_LO12_NC:
> overflow_check = false;
> ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 1, 11,
> - AARCH64_INSN_IMM_12);
> + AARCH64_INSN_IMM_12, write);
> break;
> case R_AARCH64_LDST32_ABS_LO12_NC:
> overflow_check = false;
> ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 2, 10,
> - AARCH64_INSN_IMM_12);
> + AARCH64_INSN_IMM_12, write);
> break;
> case R_AARCH64_LDST64_ABS_LO12_NC:
> overflow_check = false;
> ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 3, 9,
> - AARCH64_INSN_IMM_12);
> + AARCH64_INSN_IMM_12, write);
> break;
> case R_AARCH64_LDST128_ABS_LO12_NC:
> overflow_check = false;
> ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 4, 8,
> - AARCH64_INSN_IMM_12);
> + AARCH64_INSN_IMM_12, write);
> break;
> case R_AARCH64_TSTBR14:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 14,
> - AARCH64_INSN_IMM_14);
> + AARCH64_INSN_IMM_14, write);
> break;
> case R_AARCH64_CONDBR19:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 19,
> - AARCH64_INSN_IMM_19);
> + AARCH64_INSN_IMM_19, write);
> break;
> case R_AARCH64_JUMP26:
> case R_AARCH64_CALL26:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 26,
> - AARCH64_INSN_IMM_26);
> + AARCH64_INSN_IMM_26, write);
> if (ovf == -ERANGE) {
> val = module_emit_plt_entry(me, sechdrs, loc, &rel[i], sym);
> if (!val)
> return -ENOEXEC;
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2,
> - 26, AARCH64_INSN_IMM_26);
> + 26, AARCH64_INSN_IMM_26, write);
> }
> break;
>
> @@ -431,6 +444,30 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> return -ENOEXEC;
> }
>
> +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);
> +
> + return ret;
> +}
> +
> static inline void __init_plt(struct plt_entry *plt, unsigned long addr)
> {
> *plt = get_plt_entry(addr, plt);
> --
> 2.49.0.604.gff1f9ca942-goog
Thanks for posting the patch.
We are testing this patch using two types of kernels.
Both kernels are based on version 6.13, one is patched to use the sframe unwinder
in livepatch [1], the other is patched to not use sframe [2].
[1] https://lore.kernel.org/all/20250127213310.2496133-1-wnliu@google.com/
[2] https://lore.kernel.org/linux-arm-kernel/20250320171559.3423224-1-song@kernel.org/
For testing, we used a kpatch for arm64 [3] and the package's integration tests.
In an environment where only the livepatch patch was applied,
"module-LOADED.test" failed, but after applying this patch, it passed.
Here are the comments on the test results:[4]
[3] https://github.com/dynup/kpatch/pull/1439
[4] https://github.com/dynup/kpatch/pull/1439#issuecomment-2781731440
I just want to confirm one thing.
I think the issues this patch solves are the same as those in the previously
posted patch [5].
Am I correct in understanding that this is an improved version?
Furthermore, this patch also supports rewriting data relocation sections,
which the previously posted patch did not support.
[5] https://lore.kernel.org/linux-arm-kernel/20211103210709.31790-1-surajjs@amazon.com/
Tested-by: Toshiyuki Sato <fj6611ie@...jp.fujitsu.com>
Regards,
Toshiyuki Sato
Powered by blists - more mailing lists