linux-kernel - Re: Bug with paravirt ops and livepatches

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]

Message-ID: <alpine.LNX.2.00.1604011745370.27264@pobox.suse.cz>
Date:	Fri, 1 Apr 2016 17:46:52 +0200 (CEST)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Jiri Kosina <jikos@...nel.org>
cc:	Chris J Arges <chris.j.arges@...onical.com>, jeyu@...hat.com,
	jpoimboe@...hat.com, eugene.shatokhin@...alab.ru,
	live-patching@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	pmladek@...e.cz
Subject: Re: Bug with paravirt ops and livepatches

On Fri, 1 Apr 2016, Jiri Kosina wrote:

> On Tue, 29 Mar 2016, Jiri Kosina wrote:
> 
> > Agreed; I think we should be safe applying all the alternatives (with 
> > paravirt being really just a special case of those) to the coming module 
> > at the very last phase; they really are required only during runtime, 
> > but nothing else should be depending on them. Right? If anyone is able 
> > to come up with and counter-example, please speak up :)
> 
> So I have quickly gone through all the architectures that actually do 
> overload __weak module_finalize() by their own implementation, and except 
> for applying self-modifying code changes and registering unwind tables, 
> there doesn't seem to be any relevant heavy-lifting, that'd need to be 
> done before relocations have been written.
> 
> Is the (completely untested) sort-of-a-patch below a complete rubbish 
> (on top of current livepatching.git's for-next)?
> 
> 
> 
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 5f71aa6..c003648 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
>  	return 0;
>  }
>  
> -static int post_relocation(struct module *mod, const struct load_info *info)
> +static void post_relocation(struct module *mod, const struct load_info *info)
>  {
>  	/* Sort exception table now relocations are done. */
>  	sort_extable(mod->extable, mod->extable + mod->num_exentries);
> @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info)
>  
>  	/* Setup kallsyms-specific fields. */
>  	add_kallsyms(mod, info);
> -
> -	/* Arch-specific module finalizing. */
> -	return module_finalize(info->hdr, info->sechdrs, mod);
>  }
>  
>  /* Is this module of this name done loading?  No locks held. */
> @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err < 0)
>  		goto free_modinfo;
>  
> -	err = post_relocation(mod, info);
> -	if (err < 0)
> -		goto free_modinfo;
> +	post_relocation(mod, info);
>  
>  	flush_module_icache(mod);
>  
> @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	if (err)
>  		goto bug_cleanup;
>  
> +	/* Arch-specific module finalizing. */
> +	err = module_finalize(info->hdr, info->sechdrs, mod);
> +	if (err)
> +		goto bug_cleanup;

goto coming_cleanup;

Otherwise it looks ok. I'll give it a proper look on Monday though.

Miroslav