On Wed, Nov 06, 2024 at 08:50:41AM +0200, Mike Rapoport wrote:
> Hi Andrew,
>
> Yesterday Nathan discovered and I fixed another small issue with fineibt.
> I suspect it's too late to add this as a fixup, so here's a formal patch
> with the fix.
>
> From b31fd8493c4e1b6042776642a812690f16575b51 Mon Sep 17 00:00:00 2001
> From: "Mike Rapoport (Microsoft)" <rppt@xxxxxxxxxx>
> Date: Tue, 5 Nov 2024 10:49:57 +0200
> Subject: [PATCH] x86/alternatives: fix writable address in cfi_rewrite_endbr()
>
> Commit a159950eb69f ("x86/module: prepare module loading for ROX
> allocations of text") missed the offset that should be added to the
> writable address passed to poison_endbr() from cfi_rewrite_endbr() and
> this causes boot failures on kernels running with cfi=fineibt on
> machines that support IBT.
>
> Add required offset to wr_addr argument to fix the issue.
>
> Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> Fixes: a159950eb69f ("x86/module: prepare module loading for ROX allocations of text")
> Tested-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> ---
> arch/x86/kernel/alternative.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 3407efc26528..243843e44e89 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1241,7 +1241,7 @@ static void cfi_rewrite_endbr(s32 *start, s32 *end, struct module *mod)
> void *addr = (void *)s + *s;
> void *wr_addr = module_writable_address(mod, addr);
>
> - poison_endbr(addr+16, wr_addr, false);
> + poison_endbr(addr + 16, wr_addr + 16, false);
> }
> }
So... *sigh*. I had to rebase quite a few patches on top of this, and
while doing do I got quite annoyed at how messy all this is, so I
cleaned it all up.
Only to find out that it's all broken, even with the above fix (my ADL
will currently die when it tries to load a module).
So it's a good thing these patches got an ack from the x86 people I
suppose :-(((
Anyway, while noodling with all that, I think there's a fairly
fundamental error in all of this. The mem->rw_copy should not be a whole
second allocation, it should be a (page granular) RW alias of the
(large) ROX map.
That also gets rid of that whole copy operation.
I'm having to chase a few regressions of my own first, but after that
I'll look at reworking all this.
