[PATCH v4] powerpc: Avoid code patching freed init sections

[Michal Suchánek]

On Tue, 18 Sep 2018 10:52:09 +0200
Christophe LEROY <christophe.leroy at c-s.fr> wrote:

> 
> 
> Le 14/09/2018 à 06:22, Nicholas Piggin a écrit :
> > On Fri, 14 Sep 2018 11:14:11 +1000
> > Michael Neuling <mikey at neuling.org> wrote:
> > 
> >> This stops us from doing code patching in init sections after
> >> they've been freed.
> >>
> >> In this chain:
> >>    kvm_guest_init() ->
> >>      kvm_use_magic_page() ->
> >>        fault_in_pages_readable() ->
> >> 	 __get_user() ->
> >> 	   __get_user_nocheck() ->
> >> 	     barrier_nospec();
> >>
> >> We have a code patching location at barrier_nospec() and
> >> kvm_guest_init() is an init function. This whole chain gets
> >> inlined, so when we free the init section (hence
> >> kvm_guest_init()), this code goes away and hence should no longer
> >> be patched.
> >>
> >> We seen this as userspace memory corruption when using a memory
> >> checker while doing partition migration testing on powervm (this
> >> starts the code patching post migration via
> >> /sys/kernel/mobility/migration). In theory, it could also happen
> >> when using /sys/kernel/debug/powerpc/barrier_nospec.
> >>
> >> cc: stable at vger.kernel.org # 4.13+
> >> Signed-off-by: Michael Neuling <mikey at neuling.org>
> >>
> >> ---
> >> For stable I've marked this as v4.13+ since that's when we
> >> refactored code-patching.c but it could go back even further than
> >> that. In reality though, I think we can only hit this since the
> >> first spectre/meltdown changes.
> >>
> >> v4:
> >>   Feedback from Christophe Leroy:
> >>     - init_mem_free -> init_mem_is_free
> >>     - prlog %lx -> %px
> >>
> >> v3:
> >>   Add init_mem_free flag to avoid potential race.
> >>   Feedback from Christophe Leroy:
> >>     - use init_section_contains()
> >>     - change order of init test for performance
> >>     - use pr_debug()
> >>     - remove blank line
> >>
> >> v2:
> >>    Print when we skip an address
> >> ---
> >>   arch/powerpc/include/asm/setup.h | 1 +
> >>   arch/powerpc/lib/code-patching.c | 6 ++++++
> >>   arch/powerpc/mm/mem.c            | 2 ++
> >>   3 files changed, 9 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/setup.h
> >> b/arch/powerpc/include/asm/setup.h index 1a951b0046..1fffbba8d6
> >> 100644 --- a/arch/powerpc/include/asm/setup.h
> >> +++ b/arch/powerpc/include/asm/setup.h
> >> @@ -9,6 +9,7 @@ extern void ppc_printk_progress(char *s, unsigned
> >> short hex); 
> >>   extern unsigned int rtas_data;
> >>   extern unsigned long long memory_limit;
> >> +extern bool init_mem_is_free;
> >>   extern unsigned long klimit;
> >>   extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
> >>   
> >> diff --git a/arch/powerpc/lib/code-patching.c
> >> b/arch/powerpc/lib/code-patching.c index 850f3b8f4d..6ae2777c22
> >> 100644 --- a/arch/powerpc/lib/code-patching.c
> >> +++ b/arch/powerpc/lib/code-patching.c
> >> @@ -28,6 +28,12 @@ static int __patch_instruction(unsigned int
> >> *exec_addr, unsigned int instr, {
> >>   	int err;
> >>   
> >> +	/* Make sure we aren't patching a freed init section */
> >> +	if (init_mem_is_free && init_section_contains(exec_addr,
> >> 4)) {
> >> +		pr_debug("Skipping init section patching addr:
> >> 0x%px\n", exec_addr);
> >> +		return 0;
> >> +	}
> > 
> > What we should do is a whitelist, make sure it's only patching the
> > sections we want it to.
> > 
> > That's a bigger job when you consider modules and things too though,
> > so this looks good for now. Thanks,
> 
> What about using kernel_text_address() for it then ? It also handles 
> modules, is it more complicated than that ?

Modules are patched separately so should not need to be excluded here.

There is a different problem with modules: when the mitigation type
changes the modules are not re-patched with the new settings.

Thanks

Michal