Re: [PATCH v7] livepatch: Clear relocation targets on a module removal

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-12-14 09:40:35, Song Liu wrote:
> From: Miroslav Benes <mbenes@suse.cz>
> 
> 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