* [PATCH v2] powerpc: Avoid code patching freed init sections
@ 2018-09-12 5:20 Michael Neuling
2018-09-12 6:23 ` Christophe LEROY
0 siblings, 1 reply; 7+ messages in thread
From: Michael Neuling @ 2018-09-12 5:20 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni, mikey,
Michal Suchánek, Christophe LEROY
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.
v2:
Print when we skip an address
---
arch/powerpc/lib/code-patching.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 850f3b8f4d..68254e7f17 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -23,11 +23,33 @@
#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()) {
+ printk(KERN_DEBUG "Skipping init section patching addr: 0x%lx\n",
+ (unsigned long)patch_addr);
+ return 0;
+ }
+
__put_user_size(instr, patch_addr, 4, err);
if (err)
return err;
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-12 5:20 [PATCH v2] powerpc: Avoid code patching freed init sections Michael Neuling
@ 2018-09-12 6:23 ` Christophe LEROY
2018-09-13 0:36 ` Michael Neuling
0 siblings, 1 reply; 7+ messages in thread
From: Christophe LEROY @ 2018-09-12 6:23 UTC (permalink / raw)
To: Michael Neuling, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
Le 12/09/2018 à 07:20, Michael Neuling a écrit :
> 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.
>
> v2:
> Print when we skip an address
> ---
> arch/powerpc/lib/code-patching.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 850f3b8f4d..68254e7f17 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -23,11 +23,33 @@
> #include <asm/code-patching.h>
> #include <asm/setup.h>
>
> +
This blank line is not needed
> +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;
> +}
Can we use the existing function init_section_contains() instead of this
new function ?
> +
> +static inline bool init_freed(void)
> +{
> + return (system_state >= SYSTEM_RUNNING);
> +}
> +
I would call this function differently, for instance init_is_finished(),
because as you mentionned it doesn't exactly mean that init memory is freed.
> 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()) {
The test must be done on exec_addr, not on patch_addr, as patch_addr is
the address where the instruction as been remapped RW for allowing its
modification.
Also I think it should be tested the other way round, because the
init_freed() is a simpler test which will be false most of the time once
the system is running so it should be checked first.
> + printk(KERN_DEBUG "Skipping init section patching addr: 0x%lx\n",
Maybe use pr_debug() instead.
> + (unsigned long)patch_addr);
Please align second line as per Codying style.
> + return 0;
> + }
> +
> __put_user_size(instr, patch_addr, 4, err);
> if (err)
> return err;
>
I think it would be better to put this verification in
patch_instruction() instead, to avoid RW mapping/unmapping the
instruction to patch when we are not going to do the patching.
Christophe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-12 6:23 ` Christophe LEROY
@ 2018-09-13 0:36 ` Michael Neuling
2018-09-13 1:21 ` Tyrel Datwyler
2018-09-13 5:45 ` Christophe LEROY
0 siblings, 2 replies; 7+ messages in thread
From: Michael Neuling @ 2018-09-13 0:36 UTC (permalink / raw)
To: Christophe LEROY, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -23,11 +23,33 @@
> > #include <asm/code-patching.h>
> > #include <asm/setup.h>
> > =20
> > +
>=20
> This blank line is not needed
Ack
>=20
> > +static inline bool in_init_section(unsigned int *patch_addr)
> > +{
> > + if (patch_addr < (unsigned int *)__init_begin)
> > + return false;
> > + if (patch_addr >=3D (unsigned int *)__init_end)
> > + return false;
> > + return true;
> > +}
>=20
> Can we use the existing function init_section_contains() instead of this=
=20
> new function ?
Nice, I was looking for something like that...=20
> > +
> > +static inline bool init_freed(void)
> > +{
> > + return (system_state >=3D SYSTEM_RUNNING);
> > +}
> > +
>=20
> I would call this function differently, for instance init_is_finished(),=
=20
> because as you mentionned it doesn't exactly mean that init memory is fre=
ed.
Talking to Nick and mpe offline I think we are going to have to add a flag =
when
we free init mem rather than doing what we have now since what we have now =
has a
potential race. That change will eliminate the function entirely.
> > static int __patch_instruction(unsigned int *exec_addr, unsigned int
> > instr,
> > unsigned int *patch_addr)
> > {
> > int err;
> > =20
> > + /* Make sure we aren't patching a freed init section */
> > + if (in_init_section(patch_addr) && init_freed()) {
>=20
> The test must be done on exec_addr, not on patch_addr, as patch_addr is=
=20
> the address where the instruction as been remapped RW for allowing its=
=20
> modification.
Thanks for the catch
> Also I think it should be tested the other way round, because the=20
> init_freed() is a simpler test which will be false most of the time once=
=20
> the system is running so it should be checked first.
ok, I'll change.
> > + printk(KERN_DEBUG "Skipping init section patching addr:
> > 0x%lx\n",
>=20
> Maybe use pr_debug() instead.
Sure.
>=20
> > + (unsigned long)patch_addr);
>=20
> Please align second line as per Codying style.
Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell=
it
this out for me...
>=20
> > + return 0;
> > + }
> > +
> > __put_user_size(instr, patch_addr, 4, err);
> > if (err)
> > return err;
> >=20
>=20
> I think it would be better to put this verification in=20
> patch_instruction() instead, to avoid RW mapping/unmapping the=20
> instruction to patch when we are not going to do the patching.
If we do it there then we miss the raw_patch_intruction case.
IMHO I don't think we need to optimise this rare and non-critical path.=20
Mikey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-13 0:36 ` Michael Neuling
@ 2018-09-13 1:21 ` Tyrel Datwyler
2018-09-13 5:38 ` Christophe LEROY
2018-09-13 5:45 ` Christophe LEROY
1 sibling, 1 reply; 7+ messages in thread
From: Tyrel Datwyler @ 2018-09-13 1:21 UTC (permalink / raw)
To: Michael Neuling, Christophe LEROY, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
On 09/12/2018 05:36 PM, Michael Neuling wrote:
>
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -23,11 +23,33 @@
>>> #include <asm/code-patching.h>
>>> #include <asm/setup.h>
>>>
>>> +
>>
>> This blank line is not needed
>
> Ack
>
>>
>>> +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;
>>> +}
>>
>> Can we use the existing function init_section_contains() instead of this
>> new function ?
>
> Nice, I was looking for something like that...
>
>>> +
>>> +static inline bool init_freed(void)
>>> +{
>>> + return (system_state >= SYSTEM_RUNNING);
>>> +}
>>> +
>>
>> I would call this function differently, for instance init_is_finished(),
>> because as you mentionned it doesn't exactly mean that init memory is freed.
>
> Talking to Nick and mpe offline I think we are going to have to add a flag when
> we free init mem rather than doing what we have now since what we have now has a
> potential race. That change will eliminate the function entirely.
>
>>> 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()) {
>>
>> The test must be done on exec_addr, not on patch_addr, as patch_addr is
>> the address where the instruction as been remapped RW for allowing its
>> modification.
>
> Thanks for the catch
>
>> Also I think it should be tested the other way round, because the
>> init_freed() is a simpler test which will be false most of the time once
>> the system is running so it should be checked first.
>
> ok, I'll change.
>
>>> + printk(KERN_DEBUG "Skipping init section patching addr:
>>> 0x%lx\n",
>>
>> Maybe use pr_debug() instead.
>
> Sure.
>
>>
>>> + (unsigned long)patch_addr);
>>
>> Please align second line as per Codying style.
>
> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
> this out for me...
I suspect that the suggestion is the opening parenthesis of "(unsigned long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty sure Documentation/process/coding-style.rst is very adamant that all identation is always 8 characters and spaces should never be used, but there still seems to be a lot of places/suggestions that argument lists that spill over multiple lines should be space indented to align with the very first argument at the top level. So, I guess I'm not sure what the desire is here. Although moving to pr_debug might fit it to a single line anyways. ;)
-Tyrel
>
>>
>>> + return 0;
>>> + }
>>> +
>>> __put_user_size(instr, patch_addr, 4, err);
>>> if (err)
>>> return err;
>>>
>>
>> I think it would be better to put this verification in
>> patch_instruction() instead, to avoid RW mapping/unmapping the
>> instruction to patch when we are not going to do the patching.
>
> If we do it there then we miss the raw_patch_intruction case.
>
> IMHO I don't think we need to optimise this rare and non-critical path.
>
> Mikey
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-13 1:21 ` Tyrel Datwyler
@ 2018-09-13 5:38 ` Christophe LEROY
2018-09-13 5:48 ` Michael Neuling
0 siblings, 1 reply; 7+ messages in thread
From: Christophe LEROY @ 2018-09-13 5:38 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Neuling, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
Le 13/09/2018 à 03:21, Tyrel Datwyler a écrit :
> On 09/12/2018 05:36 PM, Michael Neuling wrote:
>>
>>>
>>>> + (unsigned long)patch_addr);
>>>
>>> Please align second line as per Codying style.
>>
>> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
>> this out for me...
>
> I suspect that the suggestion is the opening parenthesis of "(unsigned long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty sure Documentation/process/coding-style.rst is very adamant that all identation is always 8 characters and spaces should never be used, but there still seems to be a lot of places/suggestions that argument lists that spill over multiple lines should be space indented to align with the very first argument at the top level. So, I guess I'm not sure what the desire is here. Although moving to pr_debug might fit it to a single line anyways. ;)
It is exactly that, as reported by checkpatch, look at
https://patchwork.ozlabs.org/patch/968850/
Christophe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-13 0:36 ` Michael Neuling
2018-09-13 1:21 ` Tyrel Datwyler
@ 2018-09-13 5:45 ` Christophe LEROY
1 sibling, 0 replies; 7+ messages in thread
From: Christophe LEROY @ 2018-09-13 5:45 UTC (permalink / raw)
To: Michael Neuling, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
Le 13/09/2018 à 02:36, Michael Neuling a écrit :
>
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -23,11 +23,33 @@
>>> #include <asm/code-patching.h>
>>> #include <asm/setup.h>
>>>
>>> +
>>
>> This blank line is not needed
>
> Ack
>
>>
>>> +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;
>>> +}
>>
>> Can we use the existing function init_section_contains() instead of this
>> new function ?
>
> Nice, I was looking for something like that...
>
>>> +
>>> +static inline bool init_freed(void)
>>> +{
>>> + return (system_state >= SYSTEM_RUNNING);
>>> +}
>>> +
>>
>> I would call this function differently, for instance init_is_finished(),
>> because as you mentionned it doesn't exactly mean that init memory is freed.
>
> Talking to Nick and mpe offline I think we are going to have to add a flag when
> we free init mem rather than doing what we have now since what we have now has a
> potential race. That change will eliminate the function entirely.
>
>>> 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()) {
>>
>> The test must be done on exec_addr, not on patch_addr, as patch_addr is
>> the address where the instruction as been remapped RW for allowing its
>> modification.
>
> Thanks for the catch
>
>> Also I think it should be tested the other way round, because the
>> init_freed() is a simpler test which will be false most of the time once
>> the system is running so it should be checked first.
>
> ok, I'll change.
>
>>> + printk(KERN_DEBUG "Skipping init section patching addr:
>>> 0x%lx\n",
>>
>> Maybe use pr_debug() instead.
>
> Sure.
>
>>
>>> + (unsigned long)patch_addr);
>>
>> Please align second line as per Codying style.
>
> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
> this out for me...
>
>>
>>> + return 0;
>>> + }
>>> +
>>> __put_user_size(instr, patch_addr, 4, err);
>>> if (err)
>>> return err;
>>>
>>
>> I think it would be better to put this verification in
>> patch_instruction() instead, to avoid RW mapping/unmapping the
>> instruction to patch when we are not going to do the patching.
>
> If we do it there then we miss the raw_patch_intruction case.
raw_patch_instruction() can only be used during init. Once kernel memory
has been marked readonly, raw_patch_instruction() cannot be used
anymore. And mark_readonly() is called immediately after free_initmem()
Christophe
>
> IMHO I don't think we need to optimise this rare and non-critical path.
>
> Mikey
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
2018-09-13 5:38 ` Christophe LEROY
@ 2018-09-13 5:48 ` Michael Neuling
0 siblings, 0 replies; 7+ messages in thread
From: Michael Neuling @ 2018-09-13 5:48 UTC (permalink / raw)
To: Christophe LEROY, Tyrel Datwyler, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
On Thu, 2018-09-13 at 07:38 +0200, Christophe LEROY wrote:
>=20
> Le 13/09/2018 =C3=A0 03:21, Tyrel Datwyler a =C3=A9crit :
> > On 09/12/2018 05:36 PM, Michael Neuling wrote:
> > >=20
> > > >=20
> > > > > + (unsigned long)patch_addr);
> > > >=20
> > > > Please align second line as per Codying style.
> > >=20
> > > Sorry I can't see what's wrong. You're (or Cody :-P) going to have to
> > > spell it
> > > this out for me...
> >=20
> > I suspect that the suggestion is the opening parenthesis of "(unsigned
> > long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty su=
re
> > Documentation/process/coding-style.rst is very adamant that all identat=
ion
> > is always 8 characters and spaces should never be used, but there still
> > seems to be a lot of places/suggestions that argument lists that spill =
over
> > multiple lines should be space indented to align with the very first
> > argument at the top level. So, I guess I'm not sure what the desire is =
here.
> > Although moving to pr_debug might fit it to a single line anyways. ;)
>=20
> It is exactly that, as reported by checkpatch, look at=20
> https://patchwork.ozlabs.org/patch/968850/
Sweet... looks like v3 is clean
https://patchwork.ozlabs.org/patch/969241/
Mikey
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-13 5:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-12 5:20 [PATCH v2] powerpc: Avoid code patching freed init sections Michael Neuling
2018-09-12 6:23 ` Christophe LEROY
2018-09-13 0:36 ` Michael Neuling
2018-09-13 1:21 ` Tyrel Datwyler
2018-09-13 5:38 ` Christophe LEROY
2018-09-13 5:48 ` Michael Neuling
2018-09-13 5:45 ` Christophe LEROY
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.