On Fri 2023-01-06 12:01:09, Song Liu wrote:
> From: Miroslav Benes <mbenes@xxxxxxx>
>
> Josh reported a bug:
>
> When the object to be patched is a module, and that module is
> rmmod'ed and reloaded, it fails to load with:
>
> module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
> livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
> livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'
>
> The livepatch module has a relocation which references a symbol
> in the _previous_ loading of nfsd. When apply_relocate_add()
> tries to replace the old relocation with a new one, it sees that
> the previous one is nonzero and it errors out.
>
> We thus decided to reverse the relocation patching (clear all relocation
> targets on x86_64). The solution is not
> universal and is too much arch-specific, but it may prove to be simpler
> in the end.
>
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -162,56 +167,53 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
>
> switch (ELF64_R_TYPE(rel[i].r_info)) {
> case R_X86_64_NONE:
> - break;
> + continue; /* nothing to write */
> case R_X86_64_64:
> - if (*(u64 *)loc != 0)
> - goto invalid_relocation;
> - write(loc, &val, 8);
> + write_size = 8;
> break;
> case R_X86_64_32:
> - if (*(u32 *)loc != 0)
> - goto invalid_relocation;
> - write(loc, &val, 4);
> - if (val != *(u32 *)loc)
> + if (val != *(u32 *)&val)
> goto overflow;
> break;
> case R_X86_64_32S:
> - if (*(s32 *)loc != 0)
> - goto invalid_relocation;
> - write(loc, &val, 4);
> - if ((s64)val != *(s32 *)loc)
> + if ((s64)val != *(s32 *)&val)
> goto overflow;
> break;
> case R_X86_64_PC32:
> case R_X86_64_PLT32:
> - if (*(u32 *)loc != 0)
> - goto invalid_relocation;
> - val -= (u64)loc;
> - write(loc, &val, 4);
> #if 0
> - if ((s64)val != *(s32 *)loc)
> + if ((s64)val != *(s32 *)&val)
> goto overflow;
This is supposed to check the to-be-written value.
> #endif
> + val -= (u64)loc;
This is modifying the to-be-written value. It should be computed before
the overflow check.
I know that the check is not really compiled in but we should
not break it.
> break;
Otherwise, it looks fine.
Now, I agree with Miroslav that we should get an approval from x86
maintainers. Sigh, I think that I have already asked for this earlier:
!!! Please add x86@xxxxxxxxxx and linux-kernel@xxxxxxxxxxxxxxx at
minimum into CC when sending V9 !!!
The more people know about this change the better. And it is really
important to make maintainers of the touched subsystem aware of
proposed changes.
It is a good practice to add people that are printed by
./scripts/get_maintainer.pl. In this case, it is:
$> ./scripts/get_maintainer.pl arch/x86/kernel/module.c
Thomas Gleixner <tglx@xxxxxxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),authored:2/12=17%,added_lines:24/74=32%,removed_lines:5/26=19%)
Ingo Molnar <mingo@xxxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
Borislav Petkov <bp@xxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:4/12=33%)
Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
x86@xxxxxxxxxx (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
"H. Peter Anvin" <hpa@xxxxxxxxx> (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
Peter Zijlstra <peterz@xxxxxxxxxxxxx> (commit_signer:8/12=67%,authored:4/12=33%,added_lines:41/74=55%,removed_lines:8/26=31%)
Kees Cook <keescook@xxxxxxxxxxxx> (commit_signer:4/12=33%)
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> (commit_signer:3/12=25%)
"Jason A. Donenfeld" <Jason@xxxxxxxxx> (commit_signer:3/12=25%,authored:3/12=25%,removed_lines:3/26=12%)
Julian Pidancet <julian.pidancet@xxxxxxxxxx> (authored:1/12=8%,added_lines:5/74=7%,removed_lines:6/26=23%)
Ard Biesheuvel <ardb@xxxxxxxxxx> (authored:1/12=8%,removed_lines:3/26=12%)
linux-kernel@xxxxxxxxxxxxxxx (open list:X86 ARCHITECTURE (32-BIT AND 64-BIT))
Best Regards,
Petr
