linux-kernel - Re: [PATCH v5 3/9] powerpc: Prepare for moving thread_info into task

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: <459532cd-5ae3-681e-2641-4833d3abb4a3@c-s.fr>
Date:   Sun, 7 Oct 2018 05:31:35 +0000
From:   Christophe Leroy <christophe.leroy@....fr>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 3/9] powerpc: Prepare for moving thread_info into
 task_struct



On 10/06/2018 12:40 PM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@....fr> writes:
> 
>> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
>> index 47a03b9b528b..818451bf629c 100644
>> --- a/arch/powerpc/include/asm/livepatch.h
>> +++ b/arch/powerpc/include/asm/livepatch.h
>> @@ -49,7 +49,7 @@ static inline void klp_init_thread_info(struct thread_info *ti)
>>   	ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
> 
> We need to do something about this.

Oops I missed that one.

> 
> Currently the thread_info sits at the low address of the stack, and we
> use the space immediately above that as a miniature upward growing stack
> for livepatching.
> 
> If we keep the livepatch_sp in the thread_info then we need to
> initialise it somewhere when the task starts running on a stack. And I
> don't know how that works if we end up running on the emergency stack
> for example.
> 
> So I'm not sure that makes much sense.
> 
> Instead we might need to keep the livepatch_sp on the stack page at the
> base, where thread_info currently lives.
> 
> That obviously sucks because you can still overflow into it and trash
> it, but it's no worse than what we have now for livepatching.
> 
> Need to think about it some more.
> 

I think for that serie we can leave with it in the stack, as it won't be 
worst than before. Then in a future patch we can change it. I'll open an 
issue for that on github.

Then for now, I'll add the following change in this patch.

diff --git a/arch/powerpc/include/asm/livepatch.h 
b/arch/powerpc/include/asm/livepatch.h
index 47a03b9b528b..8a81d10ccc82 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -43,13 +43,14 @@ static inline unsigned long 
klp_get_ftrace_location(unsigned long faddr)
  	return ftrace_location_range(faddr, faddr + 16);
  }

-static inline void klp_init_thread_info(struct thread_info *ti)
+static inline void klp_init_thread_info(struct task_struct *p)
  {
+	struct thread_info *ti = task_thread_info(p);
  	/* + 1 to account for STACK_END_MAGIC */
-	ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
+	ti->livepatch_sp = end_of_stack(p) + 1;
  }
  #else
-static void klp_init_thread_info(struct thread_info *ti) { }
+static inline void klp_init_thread_info(struct task_struct *p) { }
  #endif /* CONFIG_LIVEPATCH */

  #endif /* _ASM_POWERPC_LIVEPATCH_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index d9d4eb2ea6c9..a12307ebb7ef 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1632,7 +1632,7 @@ int copy_thread(unsigned long clone_flags, 
unsigned long usp,
  	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
  	struct thread_info *ti = task_thread_info(p);

-	klp_init_thread_info(ti);
+	klp_init_thread_info(p);

  	/* Copy registers */
  	sp -= sizeof(struct pt_regs);
diff --git a/arch/powerpc/kernel/setup-common.c 
b/arch/powerpc/kernel/setup-common.c
index 9ca9db707bcb..8054a7b9e026 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -940,7 +940,7 @@ void __init setup_arch(char **cmdline_p)
  	/* Reserve large chunks of memory for use by CMA for KVM. */
  	kvm_cma_reserve();

-	klp_init_thread_info(&init_thread_info);
+	klp_init_thread_info(&init_task);

  	init_mm.start_code = (unsigned long)_stext;
  	init_mm.end_code = (unsigned long) _etext;

Christophe