linux-kernel - Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early

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]
Date:   Mon, 5 Nov 2018 12:05:57 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Nadav Amit <namit@...are.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "H. Peter Anvin" <hpa@...or.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andrew Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init

On Mon, Nov 5, 2018 at 11:25 AM Nadav Amit <namit@...are.com> wrote:
>
> From: Andy Lutomirski
> Sent: November 5, 2018 at 7:03:49 PM GMT
> > To: Nadav Amit <namit@...are.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>, H. Peter Anvin <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>, Kees Cook <keescook@...omium.org>, Dave Hansen <dave.hansen@...el.com>, Masami Hiramatsu <mhiramat@...nel.org>
> > Subject: Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init
> >
> >
> >
> >
> >> On Nov 5, 2018, at 9:49 AM, Nadav Amit <namit@...are.com> wrote:
> >>
> >> From: Andy Lutomirski
> >> Sent: November 5, 2018 at 5:22:32 PM GMT
> >>> To: Peter Zijlstra <peterz@...radead.org>
> >>> Cc: Nadav Amit <namit@...are.com>, Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org, x86@...nel.org, H. Peter Anvin <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>, Kees Cook <keescook@...omium.org>, Dave Hansen <dave.hansen@...el.com>, Masami Hiramatsu <mhiramat@...nel.org>
> >>> Subject: Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init
> >>>
> >>>
> >>>
> >>>>> On Nov 5, 2018, at 6:09 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> >>>>>
> >>>>> On Fri, Nov 02, 2018 at 04:29:41PM -0700, Nadav Amit wrote:
> >>>>> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> >>>>> index aac0c1f7e354..367c1d0c20a3 100644
> >>>>> --- a/arch/x86/kernel/jump_label.c
> >>>>> +++ b/arch/x86/kernel/jump_label.c
> >>>>> @@ -52,7 +52,13 @@ static void __ref __jump_label_transform(struct jump_entry *entry,
> >>>>> jmp.offset = jump_entry_target(entry) -
> >>>>>          (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
> >>>>>
> >>>>> -    if (early_boot_irqs_disabled)
> >>>>> +    /*
> >>>>> +     * As long as we are in early boot, we can use text_poke_early(), which
> >>>>> +     * is more efficient: the memory was still not marked as read-only (it
> >>>>> +     * is only marked after poking_init()). This also prevents us from using
> >>>>> +     * text_poke() before poking_init() is called.
> >>>>> +     */
> >>>>> +    if (!early_boot_done)
> >>>>>     poker = text_poke_early;
> >>>>>
> >>>>> if (type == JUMP_LABEL_JMP) {
> >>>>
> >>>> It took me a while to untangle init/maze^H^Hin.c... but I think this
> >>>> is all we need:
> >>>>
> >>>> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> >>>> index aac0c1f7e354..ed5fe274a7d8 100644
> >>>> --- a/arch/x86/kernel/jump_label.c
> >>>> +++ b/arch/x86/kernel/jump_label.c
> >>>> @@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry,
> >>>> jmp.offset = jump_entry_target(entry) -
> >>>>          (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
> >>>>
> >>>> -    if (early_boot_irqs_disabled)
> >>>> +    /*
> >>>> +     * As long as we're UP and not yet marked RO, we can use
> >>>> +     * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to
> >>>> +     * SYSTEM_SCHEDULING before going either.
> >>>> +     */
> >>>> +    if (system_state == SYSTEM_BOOTING)
> >>>>     poker = text_poke_early;
> >>>>
> >>>> if (type == JUMP_LABEL_JMP) {
> >>>
> >>> Can we move this logic into text_poke() and get rid of text_poke_early()?
> >>
> >> This will negatively affect poking of modules doing module loading, e.g.,
> >> apply_paravirt(). This can be resolved by keeping track when the module is
> >> write-protected and giving a module parameter to text_poke(). Does it worth
> >> the complexity?
> >
> > Probably not.
> >
> > OTOH, why does alternative patching need text_poke() at all? Can’t it just
> > write to the text?
>
> Good question. According to my understanding, these games of
> text_poke_early() are not needed, at least for modules (on Intel).
>
> Intel SDM 11.6 "SELF-MODIFYING CODE” says:
>
> "A write to a memory location in a code segment that is currently cached in
> the processor causes the associated cache line (or lines) to be invalidated.
> This check is based on the physical address of the instruction.”
>
> Then the manual talks about prefetched instructions, but the modules code is
> presumably not be “prefetchable” at this point. So I think it should be
> safe, but I guess that you reviewed Intel/AMD manuals better when you wrote
> sync_core().

Beats the heck out of me.

Linus, hpa, or Dave, a question for you: suppose I map some page
writably, write to it, then upgrade permissions to allow execute.
Must I force all CPUs that might execute from it without first
serializing to serialize?  I suspect this doesn't really affect user
code, but it may affect the module loader.

To be safe, shouldn't the module loader broadcast an IPI to
sync_core() everywhere after loading a module and before making it
runnable, regardless of alternative patching?

IOW, the right sequence of events probably ought to me:

1. Allocate the memory and map it.
2. Copy in the text.
3. Patch alternatives, etc.  This is logically just like (2) from an
architectural perspective -- we're just writing to memory that won't
be executed.
4. Serialize everything.
5. Run it!

>
> Anyhow, there should be a function that wraps the memcpy() to keep track
> when someone changes the text (for potential future use).
>
> Does it make sense? Do you want me to give it a spin?

Sure, I guess.  Linus, what do you think?

>
> Thanks,
> Nadav



-- 
Andy Lutomirski
AMA Capital Management, LLC