linux-kernel - Re: [RFC PATCH v3 4/6] livepatch: reuse module loader code to write relocations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Jan 2016 17:40:06 +0100 (CET)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Jessica Yu <jeyu@...hat.com>
cc:	Rusty Russell <rusty@...tcorp.com.au>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	Jonathan Corbet <corbet@....net>, linux-api@...r.kernel.org,
	live-patching@...r.kernel.org, x86@...nel.org,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [RFC PATCH v3 4/6] livepatch: reuse module loader code to write
 relocations


Hi Jessica,

I walked through the series and it looks really nice. Others have already 
pointed out the issues I also found, so only few minor things below.

First thing, could you copy&paste the information and reasoning from the 
cover letter to the changelogs where appropriate? It is very detailed and 
it would be a pity to lost it.

On Fri, 8 Jan 2016, Jessica Yu wrote:

> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index 19c099a..7312e25 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -33,8 +33,6 @@ static inline int klp_check_compiler_support(void)
>  #endif
>  	return 0;
>  }
> -int klp_write_module_reloc(struct module *mod, unsigned long type,
> -			   unsigned long loc, unsigned long value);

You left klp_write_module_reloc() in arch/s390/include/asm/livepatch.h I'm 
afraid. Anyway it would be really great if you managed to test the series 
on s390 somehow. Just to know that all the roadblocks are really gone.

> -/*
> - * external symbols are located outside the parent object (where the parent
> - * object is either vmlinux or the kmod being patched).
> - */
> -static int klp_find_external_symbol(struct module *pmod, const char *name,
> -				    unsigned long *addr)
> +static int klp_resolve_symbols(Elf_Shdr *relsec, struct module *pmod)
>  {
> -	const struct kernel_symbol *sym;
> +	int i, len, ret = 0;
> +	Elf_Rela *relas;
> +	Elf_Sym *sym;
> +	char *symname, *sym_objname;
>  
> -	/* first, check if it's an exported symbol */
> -	preempt_disable();
> -	sym = find_symbol(name, NULL, NULL, true, true);
> -	if (sym) {
> -		*addr = sym->value;
> -		preempt_enable();
> -		return 0;
> +	relas = (Elf_Rela *) relsec->sh_addr;
> +	/* For each rela in this .klp.rel. section */
> +	for (i = 0; i < relsec->sh_size / sizeof(Elf_Rela); i++) {
> +		sym = pmod->core_symtab + ELF_R_SYM(relas[i].r_info);
> +		symname = pmod->core_strtab + sym->st_name;

Maybe it would be better to use pmod->symtab and pmod->strtab everywhere. 
It should be the same, but core_* versions are only helpers used in 
load_module and friends. There is even a comment in 
include/linux/module.h.

	/*
	 * We keep the symbol and string tables for kallsyms.
	 * The core_* fields below are temporary, loader-only (they
	 * could really be discarded after module init).
	 */

We should respect that.

Thanks,
Miroslav