On Wed 2022-12-14 09:40:35, 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/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -739,6 +739,67 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> return 0;
> }
>
> +#ifdef CONFIG_LIVEPATCH
> +void clear_relocate_add(Elf64_Shdr *sechdrs,
> + const char *strtab,
> + unsigned int symindex,
> + unsigned int relsec,
> + struct module *me)
> +{
> + unsigned int i;
> + Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> + Elf64_Sym *sym;
> + unsigned long *location;
> + const char *symname;
> + u32 *instruction;
> +
> + pr_debug("Clearing ADD relocate section %u to %u\n", relsec,
> + sechdrs[relsec].sh_info);
> +
> + for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rela); i++) {
> + location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
> + + rela[i].r_offset;
> + sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
> + + ELF64_R_SYM(rela[i].r_info);
> + symname = me->core_kallsyms.strtab
> + + sym->st_name;
> +
> + if (ELF64_R_TYPE(rela[i].r_info) != R_PPC_REL24)
> + continue;
Is it OK to continue?
IMHO, we should at least warn here. It means that the special elf
section contains a relocation that we are not able to clear. It will
most likely blow up when we try to load the livepatched module
again.
> + /*
> + * reverse the operations in apply_relocate_add() for case
> + * R_PPC_REL24.
> + */
> + if (sym->st_shndx != SHN_UNDEF &&
> + sym->st_shndx != SHN_LIVEPATCH)
> + continue;
Same here. IMHO, we should warn when the section contains something
that we are not able to clear.
> + /* skip mprofile and ftrace calls, same as restore_r2() */
> + if (is_mprofile_ftrace_call(symname))
> + continue;
Is this correct? restore_r2() returns "1" in this case. As a result
apply_relocate_add() returns immediately with -ENOEXEC. IMHO, we
should print a warning and return as well.
> + instruction = (u32 *)location;
> + /* skip sibling call, same as restore_r2() */
> + if (!instr_is_relative_link_branch(ppc_inst(*instruction)))
> + continue;
Same here. restore_r2() returns '1' in this case...
> +
> + instruction += 1;
> + /*
> + * Patch location + 1 back to NOP so the next
> + * apply_relocate_add() call (reload the module) will not
> + * fail the sanity check in restore_r2():
> + *
> + * if (*instruction != PPC_RAW_NOP()) {
> + * pr_err(...);
> + * return 0;
> + * }
> + */
> + patch_instruction(instruction, ppc_inst(PPC_RAW_NOP()));
> + }
This seems incomplete. The above code reverts patch_instruction() called
from restore_r2(). But there is another patch_instruction() called in
apply_relocate_add() for case R_PPC_REL24. IMHO, we should revert this
as well.
> +}
> +#endif
IMHO, this approach is really bad. The function is not maintainable.
It will be very hard to keep it in sync with apply_relocate_add().
And all the mistakes are just a proof.
IMHO, the only sane way is to avoid the code duplication.
> #ifdef CONFIG_DYNAMIC_FTRACE
> int module_trampoline_target(struct module *mod, unsigned long addr,
> unsigned long *target)
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -261,6 +261,41 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
> return 0;
> }
>
> +static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> + const char *shstrtab, const char *strtab,
> + unsigned int symndx, unsigned int secndx,
> + const char *objname, bool apply)
> +{
> + int cnt, ret;
> + char sec_objname[MODULE_NAME_LEN];
> + Elf_Shdr *sec = sechdrs + secndx;
> +
> + /*
> + * Format: .klp.rela.sec_objname.section_name
> + * See comment in klp_resolve_symbols() for an explanation
> + * of the selected field width value.
> + */
> + cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> + sec_objname);
> + if (cnt != 1) {
> + pr_err("section %s has an incorrectly formatted name\n",
> + shstrtab + sec->sh_name);
> + return -EINVAL;
> + }
> +
> + if (strcmp(objname ? objname : "vmlinux", sec_objname))
> + return 0;
> +
> + ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> + if (ret)
> + return ret;
We do not need to call klp_resolve_symbols() when clearing the relocations.
> +
> + if (apply)
> + return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
Please, add an empty line here.
> + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return 0;
> +}
> +
> /*
> * At a high-level, there are two types of klp relocation sections: those which
> * reference symbols which live in vmlinux; and those which reference symbols
Please, keep these comments above klp_write_section_relocs().
It is the function that implements these details and it is
called from more locations.
> @@ -289,31 +324,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> unsigned int symndx, unsigned int secndx,
> const char *objname)
> {
> - int cnt, ret;
> - char sec_objname[MODULE_NAME_LEN];
> - Elf_Shdr *sec = sechdrs + secndx;
> -
> - /*
> - * Format: .klp.rela.sec_objname.section_name
> - * See comment in klp_resolve_symbols() for an explanation
> - * of the selected field width value.
> - */
> - cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
> - sec_objname);
> - if (cnt != 1) {
> - pr_err("section %s has an incorrectly formatted name\n",
> - shstrtab + sec->sh_name);
> - return -EINVAL;
> - }
> -
> - if (strcmp(objname ? objname : "vmlinux", sec_objname))
> - return 0;
> -
> - ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> - if (ret)
> - return ret;
> -
> - return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> + return klp_write_section_relocs(pmod, sechdrs, shstrtab, strtab, symndx,
> + secndx, objname, true);
> }
>
> /*
Best Regards,
Petr
