| 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: <CADBMgpyJLM91ZaPZQmzKoDghxpVvsfXjAe83k_tnNAkWnu_Rsw@mail.gmail.com>
Date: Wed, 23 Apr 2025 15:59:37 -0700
From: Dylan Hatch <dylanbhatch@...gle.com>
To: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.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>
Subject: Re: [PATCH v2 2/2] arm64/module: Use text-poke API for late relocations.
Hi Toshiyuki,
On Tue, Apr 22, 2025 at 11:05 PM Toshiyuki Sato (Fujitsu)
<fj6611ie@...itsu.com> wrote:
>
> 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
Thanks for the help with testing here.
> 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?
I wasn't aware the patch [5] existed, thanks for the link.
> Furthermore, this patch also supports rewriting data relocation sections,
> which the previously posted patch did not support.
Yes, this is correct. From this perspective, this patch is an improvement.
>
> [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
>
Thanks,
Dylan
Powered by blists - more mailing lists