Re: [RFC PATCH 5/7] x86/module: perpare module loading for ROX allocations of text
On Mon, Apr 15, 2024 at 12:43:16PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 11, 2024 at 07:05:24PM +0300, Mike Rapoport wrote:
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 45a280f2161c..b4d6868df573 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
>
> > @@ -504,17 +513,17 @@ void __init_or_module noinline
> > apply_alternatives(struct alt_instr *start,
> > * patch if feature is *NOT* present.
> > */
> > if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> > - optimize_nops_inplace(instr, a->instrlen);
> > + optimize_nops_inplace(wr_instr, a->instrlen);
> > continue;
> > }
> >
> > - DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl:
> > (%px, len: %d) flags: 0x%x",
> > + DPRINTK(ALT, "feat: %d*32+%d, old: (%px (%px) len: %d), repl:
> > (%px (%px), len: %d) flags: 0x%x",
> > a->cpuid >> 5,
> > a->cpuid & 0x1f,
> > - instr, instr, a->instrlen,
> > - replacement, a->replacementlen, a->flags);
> > + instr, wr_instr, a->instrlen,
> > + replacement, wr_replacement, a->replacementlen,
> > a->flags);
>
> I think this, and
I've found printing both address handy when I debugged it, but no strong
feelings here.
> >
> > - memcpy(insn_buff, replacement, a->replacementlen);
> > + memcpy(insn_buff, wr_replacement, a->replacementlen);
> > insn_buff_sz = a->replacementlen;
> >
> > if (a->flags & ALT_FLAG_DIRECT_CALL) {
> > @@ -528,11 +537,11 @@ void __init_or_module noinline
> > apply_alternatives(struct alt_instr *start,
> >
> > apply_relocation(insn_buff, a->instrlen, instr, replacement,
> > a->replacementlen);
> >
> > - DUMP_BYTES(ALT, instr, a->instrlen, "%px: old_insn: ", instr);
> > + DUMP_BYTES(ALT, wr_instr, a->instrlen, "%px: old_insn: ",
> > instr);
>
> this, want to remain as is.
here wr_instr is the buffer to dump:
DUMP_BYTES(type, buf, len, fmt, args...)
rather than an address, which remained 'instr'.
> > DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:
> > rpl_insn: ", replacement);
> > DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ",
> > instr);
> >
> > - text_poke_early(instr, insn_buff, insn_buff_sz);
> > + text_poke_early(wr_instr, insn_buff, insn_buff_sz);
> > }
> >
> > kasan_enable_current();
>
> The rationale being that we then print an address that can be correlated
> to the kernel image (provided one either kills kaslr or adjusts for it).
--
Sincerely yours,
Mike.
Re: [RFC PATCH 5/7] x86/module: perpare module loading for ROX allocations of text
On Thu, Apr 11, 2024 at 07:05:24PM +0300, Mike Rapoport wrote:
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 45a280f2161c..b4d6868df573 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -504,17 +513,17 @@ void __init_or_module noinline
> apply_alternatives(struct alt_instr *start,
>* patch if feature is *NOT* present.
>*/
> if (!boot_cpu_has(a->cpuid) == !(a->flags & ALT_FLAG_NOT)) {
> - optimize_nops_inplace(instr, a->instrlen);
> + optimize_nops_inplace(wr_instr, a->instrlen);
> continue;
> }
>
> - DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl:
> (%px, len: %d) flags: 0x%x",
> + DPRINTK(ALT, "feat: %d*32+%d, old: (%px (%px) len: %d), repl:
> (%px (%px), len: %d) flags: 0x%x",
> a->cpuid >> 5,
> a->cpuid & 0x1f,
> - instr, instr, a->instrlen,
> - replacement, a->replacementlen, a->flags);
> + instr, wr_instr, a->instrlen,
> + replacement, wr_replacement, a->replacementlen,
> a->flags);
I think this, and
>
> - memcpy(insn_buff, replacement, a->replacementlen);
> + memcpy(insn_buff, wr_replacement, a->replacementlen);
> insn_buff_sz = a->replacementlen;
>
> if (a->flags & ALT_FLAG_DIRECT_CALL) {
> @@ -528,11 +537,11 @@ void __init_or_module noinline
> apply_alternatives(struct alt_instr *start,
>
> apply_relocation(insn_buff, a->instrlen, instr, replacement,
> a->replacementlen);
>
> - DUMP_BYTES(ALT, instr, a->instrlen, "%px: old_insn: ", instr);
> + DUMP_BYTES(ALT, wr_instr, a->instrlen, "%px: old_insn: ",
> instr);
this, want to remain as is.
> DUMP_BYTES(ALT, replacement, a->replacementlen, "%px:
> rpl_insn: ", replacement);
> DUMP_BYTES(ALT, insn_buff, insn_buff_sz, "%px: final_insn: ",
> instr);
>
> - text_poke_early(instr, insn_buff, insn_buff_sz);
> + text_poke_early(wr_instr, insn_buff, insn_buff_sz);
> }
>
> kasan_enable_current();
The rationale being that we then print an address that can be correlated
to the kernel image (provided one either kills kaslr or adjusts for it).
Re: [RFC PATCH 5/7] x86/module: perpare module loading for ROX allocations of text
On Fri, Apr 12, 2024 at 11:08:00AM +0200, Ingo Molnar wrote:
>
> * Mike Rapoport wrote:
>
> > for (s = start; s < end; s++) {
> > void *addr = (void *)s + *s;
> > + void *wr_addr = addr + module_writable_offset(mod, addr);
>
> So instead of repeating this pattern in a dozen of places, why not use a
> simpler method:
>
> void *wr_addr = module_writable_address(mod, addr);
>
> or so, since we have to pass 'addr' to the module code anyway.
Agree.
> The text patching code is pretty complex already.
>
> Thanks,
>
> Ingo
--
Sincerely yours,
Mike.
Re: [RFC PATCH 5/7] x86/module: perpare module loading for ROX allocations of text
* Mike Rapoport wrote:
> for (s = start; s < end; s++) {
> void *addr = (void *)s + *s;
> + void *wr_addr = addr + module_writable_offset(mod, addr);
So instead of repeating this pattern in a dozen of places, why not use a
simpler method:
void *wr_addr = module_writable_address(mod, addr);
or so, since we have to pass 'addr' to the module code anyway.
The text patching code is pretty complex already.
Thanks,
Ingo
[RFC PATCH 5/7] x86/module: perpare module loading for ROX allocations of text
From: "Mike Rapoport (IBM)"
When module text memory will be allocated with ROX permissions, the
memory at the actual address where the module will live will contain
invalid instructions and there will be a writable copy that contains the
actual module code.
Update relocations and alternatives patching to deal with it.
Signed-off-by: Mike Rapoport (IBM)
---
arch/um/kernel/um_arch.c | 11 ++-
arch/x86/entry/vdso/vma.c | 3 +-
arch/x86/include/asm/alternative.h | 14 +--
arch/x86/kernel/alternative.c | 152 +
arch/x86/kernel/ftrace.c | 41 +---
arch/x86/kernel/module.c | 17 ++--
6 files changed, 140 insertions(+), 98 deletions(-)
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index c5ca89e62552..5183c955974e 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -437,24 +437,25 @@ void __init arch_cpu_finalize_init(void)
os_check_bugs();
}
-void apply_seal_endbr(s32 *start, s32 *end)
+void apply_seal_endbr(s32 *start, s32 *end, struct module *mod)
{
}
-void apply_retpolines(s32 *start, s32 *end)
+void apply_retpolines(s32 *start, s32 *end, struct module *mod)
{
}
-void apply_returns(s32 *start, s32 *end)
+void apply_returns(s32 *start, s32 *end, struct module *mod)
{
}
void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
- s32 *start_cfi, s32 *end_cfi)
+ s32 *start_cfi, s32 *end_cfi, struct module *mod)
{
}
-void apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+void apply_alternatives(struct alt_instr *start, struct alt_instr *end,
+ struct module *mod)
{
}
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 6d83ceb7f1ba..31412adef5d2 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -51,7 +51,8 @@ int __init init_vdso_image(const struct vdso_image *image)
apply_alternatives((struct alt_instr *)(image->data + image->alt),
(struct alt_instr *)(image->data + image->alt +
- image->alt_len));
+ image->alt_len),
+ NULL);
return 0;
}
diff --git a/arch/x86/include/asm/alternative.h
b/arch/x86/include/asm/alternative.h
index fcd20c6dc7f9..6f4b0776fc89 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -96,16 +96,16 @@ extern struct alt_instr __alt_instructions[],
__alt_instructions_end[];
* instructions were patched in already:
*/
extern int alternatives_patched;
+struct module;
extern void alternative_instructions(void);
-extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
-extern void apply_retpolines(s32 *start, s32 *end);
-extern void apply_returns(s32 *start, s32 *end);
-extern void apply_seal_endbr(s32 *start, s32 *end);
+extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end,
+ struct module *mod);
+extern void apply_retpolines(s32 *start, s32 *end, struct module *mod);
+extern void apply_returns(s32 *start, s32 *end, struct module *mod);
+extern void apply_seal_endbr(s32 *start, s32 *end, struct module *mod);
extern void apply_fineibt(s32 *start_retpoline, s32 *end_retpoine,
- s32 *start_cfi, s32 *end_cfi);
-
-struct module;
+ s32 *start_cfi, s32 *end_cfi, struct module *mod);
struct callthunk_sites {
s32 *call_start, *call_end;
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 45a280f2161c..b4d6868df573 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -462,7 +462,8 @@ static int alt_replace_call(u8 *instr, u8 *insn_buff,
struct alt_instr *a)
* to refetch changed I$ lines.
*/
void __init_or_module noinline apply_alternatives(struct alt_instr *start,
- struct alt_instr *end)
+ struct alt_instr *end,
+ struct module *mod)
{
struct alt_instr *a;
u8 *instr, *replacement;
@@ -490,10 +491,18 @@ void __init_or_module noinline apply_alternatives(struct
alt_instr *start,
* order.
*/
for (a = start; a < end; a++) {
+ unsigned long ins_offs, repl_offs;
int insn_buff_sz = 0;
+ u8 *wr_instr, *wr_replacement;
instr = (u8 *)>instr_offset + a->instr_offset;
+ ins_offs = module_writable_offset(mod, instr);
+ wr_instr = instr + ins_offs;
+
replacement = (u8 *)>repl_offset + a->repl_offset;
+ repl_offs = module_writable_offset(mod, replacement);
+ wr_replacement = replacement +
