Re: [PATCH] powerpc: Avoid code patching freed init sections
On Mon, Sep 10, 2018 at 08:05:38PM +1000, Michael Neuling wrote: > > > > + /* Make sure we aren't patching a freed init section */ > > > + if (in_init_section(patch_addr) && init_freed()) > > > + return 0; > > > + > > > > Do we even need the init_freed() check? > > Maybe not. If userspace isn't up, then maybe it's ok to skip. Isn't this same function used for patching asm feature sections? It's not OK to skip patching them in init code. > > What user input can we process in init-only code? > > See the stack trace in the commit message. It's a weird case for KVM guests in > KVM PR mode. The fault_in_pages_readable (formerly __get_user) there isn't actually reading userspace, it's just a way of doing a load with a convenient way to handle it if it traps. Paul.
Re: [PATCH] powerpc: Avoid code patching freed init sections
On Mon, 10 Sep 2018 12:16:35 +0200 Christophe LEROY wrote: > Le 10/09/2018 à 12:05, Michael Neuling a écrit : > > > >>> + /* Make sure we aren't patching a freed init section */ > >>> + if (in_init_section(patch_addr) && init_freed()) > >>> + return 0; > >>> + > >> > >> Do we even need the init_freed() check? > > > > Maybe not. If userspace isn't up, then maybe it's ok to skip. > > Euh ... Do you mean you'll skip all patches into init functions ? > But code patching is not only for meltdown/spectrum workarounds, some > of the patchings might be needed for the init functions themselves. Some stuff like cpu feature tests have an early variant that does not need patching but maybe not everything has. and some stuff like lwsync might be also expanded from some macros or inlines and may be needed in the init code. It might be questionable to rely on it getting patched, though. Hard to tell without seeing what is actually patched where. Thanks Michal
Re: [PATCH] powerpc: Avoid code patching freed init sections
Le 10/09/2018 à 12:05, Michael Neuling a écrit : + /* Make sure we aren't patching a freed init section */ + if (in_init_section(patch_addr) && init_freed()) + return 0; + Do we even need the init_freed() check? Maybe not. If userspace isn't up, then maybe it's ok to skip. Euh ... Do you mean you'll skip all patches into init functions ? But code patching is not only for meltdown/spectrum workarounds, some of the patchings might be needed for the init functions themselves. Christophe What user input can we process in init-only code? See the stack trace in the commit message. It's a weird case for KVM guests in KVM PR mode. That's the only case I can found so far. Also it would be nice to write the function+offset of the skipped patch location into the kernel log. OK. I'll update. Mikey
Re: [PATCH] powerpc: Avoid code patching freed init sections
> > + /* Make sure we aren't patching a freed init section */ > > + if (in_init_section(patch_addr) && init_freed()) > > + return 0; > > + > > Do we even need the init_freed() check? Maybe not. If userspace isn't up, then maybe it's ok to skip. > What user input can we process in init-only code? See the stack trace in the commit message. It's a weird case for KVM guests in KVM PR mode. That's the only case I can found so far. > Also it would be nice to write the function+offset of the skipped patch > location into the kernel log. OK. I'll update. Mikey
Re: [PATCH] powerpc: Avoid code patching freed init sections
> > 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. > > Which means it affects all maintained stable trees because the > spectre/meltdown changes are backported. Yep we this this on SLES12 SP3 Mikey
Re: [PATCH] powerpc: Avoid code patching freed init sections
On Mon, 10 Sep 2018 15:44:05 +1000
Michael Neuling 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.
>
> With this patch there is a small change of a race if we code patch
> between the init section being freed and setting SYSTEM_RUNNING (in
> kernel_init()) but that seems like an impractical time and small
> window for any code patching to occur.
>
> cc: sta...@vger.kernel.org # 4.13+
> Signed-off-by: Michael Neuling
>
> ---
> 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.
> ---
> arch/powerpc/lib/code-patching.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c index 850f3b8f4d..a2bc08bfd8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -23,11 +23,30 @@
> #include
> #include
>
> +
> +static inline bool in_init_section(unsigned int *patch_addr)
> +{
> + if (patch_addr < (unsigned int *)__init_begin)
> + return false;
> + if (patch_addr >= (unsigned int *)__init_end)
> + return false;
> + return true;
> +}
> +
> +static inline bool init_freed(void)
> +{
> + return (system_state >= SYSTEM_RUNNING);
> +}
> +
> static int __patch_instruction(unsigned int *exec_addr, unsigned int
> instr, unsigned int *patch_addr)
> {
> int err;
>
> + /* Make sure we aren't patching a freed init section */
> + if (in_init_section(patch_addr) && init_freed())
> + return 0;
> +
Do we even need the init_freed() check?
What user input can we process in init-only code?
Also it would be nice to write the function+offset of the skipped patch
location into the kernel log.
Thanks
Michal
Re: [PATCH] powerpc: Avoid code patching freed init sections
On Mon, 10 Sep 2018 15:44:05 +1000 Michael Neuling 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. > > With this patch there is a small change of a race if we code patch > between the init section being freed and setting SYSTEM_RUNNING (in > kernel_init()) but that seems like an impractical time and small > window for any code patching to occur. > > cc: sta...@vger.kernel.org # 4.13+ > Signed-off-by: Michael Neuling > > --- > 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. Which means it affects all maintained stable trees because the spectre/meltdown changes are backported. Thanks Michal
