[PATCH] powerpc: Avoid code patching freed init sections

[PATCH] powerpc: Avoid code patching freed init sections

[Michael Neuling]

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: stable@vger.kernel.org # 4.13+
Signed-off-by: Michael Neuling <mikey@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.
---
 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 <asm/code-patching.h>
 #include <asm/setup.h>
 
+
+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;
+
 	__put_user_size(instr, patch_addr, 4, err);
 	if (err)
 		return err;
-- 
2.17.1

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

[Michal Suchánek]

On Mon, 10 Sep 2018 15:44:05 +1000
Michael Neuling <mikey@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.
> 
> 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: stable@vger.kernel.org # 4.13+
> Signed-off-by: Michael Neuling <mikey@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.

Which means it affects all maintained stable trees because the
spectre/meltdown changes are backported.

Thanks

Michal

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

[Michal Suchánek]

On Mon, 10 Sep 2018 15:44:05 +1000
Michael Neuling <mikey@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.
> 
> 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: stable@vger.kernel.org # 4.13+
> Signed-off-by: Michael Neuling <mikey@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.
> ---
>  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 <asm/code-patching.h>
>  #include <asm/setup.h>
>  
> +
> +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

[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.
>=20
> Which means it affects all maintained stable trees because the
> spectre/meltdown changes are backported.

Yep we this this on SLES12 SP3=20

Mikey

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

[Michael Neuling]

> > +	/* Make sure we aren't patching a freed init section */
> > +	if (in_init_section(patch_addr) && init_freed())
> > +		return 0;
> > +
>=20
> 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.=20

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

[Christophe LEROY]

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

[Michal Suchánek]

On Mon, 10 Sep 2018 12:16:35 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 10/09/2018 =C3=A0 12:05, Michael Neuling a =C3=A9crit=C2=A0:
> >  =20
> >>> +	/* Make sure we aren't patching a freed init section */
> >>> +	if (in_init_section(patch_addr) && init_freed())
> >>> +		return 0;
> >>> + =20
> >>
> >> Do we even need the init_freed() check? =20
> >=20
> > Maybe not.  If userspace isn't up, then maybe it's ok to skip. =20
>=20
> 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

[Paul Mackerras]

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.