| 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: <CADBMgpzrQs=U8xXbD_Em5=P1=FnqtcC4uO0jDHqkF19CPng3Kg@mail.gmail.com>
Date: Tue, 3 Jun 2025 10:25:07 -0700
From: Dylan Hatch <dylanbhatch@...gle.com>
To: Will Deacon <will@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>, Ard Biesheuvel <ardb@...nel.org>,
Sami Tolvanen <samitolvanen@...gle.com>, Geert Uytterhoeven <geert@...ux-m68k.org>,
Song Liu <song@...nel.org>, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, Roman Gushchin <roman.gushchin@...ux.dev>,
Toshiyuki Sato <fj6611ie@...jp.fujitsu.com>
Subject: Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.
On Tue, Jun 3, 2025 at 8:13 AM Will Deacon <will@...nel.org> wrote:
>
> Hey Dylan,
>
> On Fri, May 30, 2025 at 05:11:00PM -0700, Dylan Hatch wrote:
> > On Fri, May 30, 2025 at 7:13 AM Will Deacon <will@...nel.org> wrote:
> > >
> > > and this would be:
> > >
> > > WRITE_PLACE(place, cpu_to_le32(insn), me);
> > >
> >
> > I'm seeing this part give a build error:
> >
> > arch/arm64/kernel/module.c:158:2: error: cannot take the address of an
> > rvalue of type '__le32' (aka 'unsigned int')
> > 158 | WRITE_PLACE(place, cpu_to_le32(insn), me);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/arm64/kernel/module.c:56:28: note: expanded from macro 'WRITE_PLACE'
> > 56 | aarch64_insn_copy(place, &(val),
> > sizeof(*place)); \
> > | ^ ~~~
> >
> > I can't think of a clean way to get around this and still keep a
> > combined write helper. Setting an intermediate __le32 in the
> > reloc_insn_* functions would work but we were trying to avoid that.
> > Setting an intermediate value inside WRITE_PLACE would also work but
> > then (I think) it won't work for the data relocations because we'd be
> > converting a signed into unsigned value. Making WRITE_PLACE a function
> > instead of a macro also fixes the rvalue problem but then the args
> > 'place' and 'val' have to be of a fixed type so we can't do the
> > typecasting on 'place' and 'val' has the same signed/unsigned value
> > problem.
> >
> > Do you have a suggestion here? In the meantime I can send a v6 that
> > uses an intermediate __le32 for the instruction relocations.
>
> Sorry for the slow reply -- I see you already sent a v6. I think we
> could add a temporary in the macro. Diff below (on top of your v6). WDYT?
>
> Will
>
> --->8
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 862f6d50ab00..40148d2725ce 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -50,10 +50,12 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
> }
>
> #define WRITE_PLACE(place, val, mod) do { \
> + __typeof__(val) __val = (val); \
> + \
> if (mod->state == MODULE_STATE_UNFORMED) \
> - *(place) = val; \
> + *(place) = __val; \
> else \
> - aarch64_insn_copy(place, &(val), sizeof(*place)); \
> + aarch64_insn_copy(place, &(__val), sizeof(*place)); \
> } while (0)
>
> static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
> @@ -128,7 +130,6 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
> u64 imm;
> s64 sval;
> u32 insn = le32_to_cpu(*place);
> - __le32 le_insn;
>
> sval = do_reloc(op, place, val);
> imm = sval >> lsb;
> @@ -156,8 +157,7 @@ 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);
> - le_insn = cpu_to_le32(insn);
> - WRITE_PLACE(place, le_insn, me);
> + WRITE_PLACE(place, cpu_to_le32(insn), me);
>
> if (imm > U16_MAX)
> return -ERANGE;
> @@ -172,7 +172,6 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
> 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);
> @@ -184,8 +183,7 @@ 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);
> - le_insn = cpu_to_le32(insn);
> - WRITE_PLACE(place, le_insn, me);
> + WRITE_PLACE(place, cpu_to_le32(insn), me);
>
> /*
> * Extract the upper value bits (including the sign bit) and
> @@ -207,7 +205,6 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> __le32 *place, u64 val, struct module *me)
> {
> u32 insn;
> - __le32 le_insn;
>
> if (!is_forbidden_offset_for_adrp(place))
> return reloc_insn_imm(RELOC_OP_PAGE, place, val, 12, 21,
> @@ -227,8 +224,7 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> AARCH64_INSN_BRANCH_NOLINK);
> }
>
> - le_insn = cpu_to_le32(insn);
> - WRITE_PLACE(place, le_insn, me);
> + WRITE_PLACE(place, cpu_to_le32(insn), me);
> return 0;
> }
>
>
This looks good to me. I can send a v7 with these changes.
Thanks,
Dylan
Powered by blists - more mailing lists