From: catalin.marinas@arm.com (Catalin Marinas) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Date: Tue, 6 Sep 2016 11:27:42 +0100 [thread overview] Message-ID: <20160906102741.GF19605@e104818-lin.cambridge.arm.com> (raw) In-Reply-To: <20160905172038.GC27305@leverpostej> On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote: > On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote: > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > +#define RESERVED_TTBR0_SIZE (PAGE_SIZE) > > +#else > > +#define RESERVED_TTBR0_SIZE (0) > > +#endif > > I was going to suggest that we use the empty_zero_page, which we can > address with an adrp, because I had forgotten that we need to generate > the *physical* address. > > It would be good if we could have a description of why we need the new > reserved page somewhere in the code. I'm sure I won't be the only one > tripped up by this. > > It would be possible to use the existing empty_zero_page, if we're happy > to have a MOVZ; MOVK; MOVK; MOVK sequence that we patch at boot-time. > That could be faster than an MRS on some implementations. I was trying to keep the number of instructions to a minimum in preference to potentially slightly faster sequence (I haven't done any benchmarks). On ARMv8.1+ implementations, we just end up with more nops. We could also do an ldr from a PC-relative address, it's one instruction and it may not be (significantly) slower than MRS + ADD. > > +static inline void uaccess_ttbr0_enable(void) > > +{ > > + unsigned long flags; > > + > > + /* > > + * Disable interrupts to avoid preemption and potential saved > > + * TTBR0_EL1 updates between reading the variable and the MSR. > > + */ > > + local_irq_save(flags); > > + write_sysreg(current_thread_info()->ttbr0, ttbr0_el1); > > + isb(); > > + local_irq_restore(flags); > > +} > > I don't follow what problem this actually protects us against. In the > case of preemption everything should be saved+restored transparently, or > things would go wrong as soon as we enable IRQs anyway. > > Is this a hold-over from a percpu approach rather than the > current_thread_info() approach? If we get preempted between reading current_thread_info()->ttbr0 and writing TTBR0_EL1, a series of context switches could lead to the update of the ASID part of ttbr0. The actual MSR would store an old ASID in TTBR0_EL1. > > +#else > > +static inline void uaccess_ttbr0_disable(void) > > +{ > > +} > > + > > +static inline void uaccess_ttbr0_enable(void) > > +{ > > +} > > +#endif > > I think that it's better to drop the ifdef and add: > > if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN)) > return; > > ... at the start of each function. GCC should optimize the entire thing > away when not used, but we'll get compiler coverage regardless, and > therefore less breakage. All the symbols we required should exist > regardless. The reason for this is that thread_info.ttbr0 is conditionally defined. I don't think the compiler would ignore it. > > .macro uaccess_enable, tmp1, tmp2 > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > +alternative_if_not ARM64_HAS_PAN > > + save_and_disable_irq \tmp2 // avoid preemption > > + uaccess_ttbr0_enable \tmp1 > > + restore_irq \tmp2 > > +alternative_else > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > +alternative_endif > > +#endif > > How about something like: > > .macro alternative_endif_else_nop > alternative_else > .rept ((662b-661b) / 4) > nop > .endr > alternative_endif > .endm > > So for the above we could have: > > alternative_if_not ARM64_HAS_PAN > save_and_disable_irq \tmp2 > uaccess_ttbr0_enable \tmp1 > restore_irq \tmp2 > alternative_endif_else_nop > > I'll see about spinning a patch, or discovering why that happens to be > broken. This looks better. Minor comment, I would actually name the ending statement alternative_else_nop_endif to match the order in which you'd normally write them. > > * tables again to remove any speculatively loaded cache lines. > > */ > > mov x0, x25 > > - add x1, x26, #SWAPPER_DIR_SIZE > > + add x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE > > dmb sy > > bl __inval_cache_range > > > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > > index 659963d40bb4..fe393ccf9352 100644 > > --- a/arch/arm64/kernel/vmlinux.lds.S > > +++ b/arch/arm64/kernel/vmlinux.lds.S > > @@ -196,6 +196,11 @@ SECTIONS > > swapper_pg_dir = .; > > . += SWAPPER_DIR_SIZE; > > > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > + reserved_ttbr0 = .; > > + . += PAGE_SIZE; > > +#endif > > Surely RESERVED_TTBR0_SIZE, as elsewhere? I'll try to move it somewhere where it can be included in vmlinux.lds.S (I can probably include cpufeature.h directly). -- Catalin
WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com> To: Mark Rutland <mark.rutland@arm.com> Cc: Kees Cook <keescook@chromium.org>, kernel-hardening@lists.openwall.com, Will Deacon <will.deacon@arm.com>, AKASHI Takahiro <takahiro.akashi@linaro.org>, James Morse <james.morse@arm.com>, linux-arm-kernel@lists.infradead.org Subject: [kernel-hardening] Re: [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Date: Tue, 6 Sep 2016 11:27:42 +0100 [thread overview] Message-ID: <20160906102741.GF19605@e104818-lin.cambridge.arm.com> (raw) In-Reply-To: <20160905172038.GC27305@leverpostej> On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote: > On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote: > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > +#define RESERVED_TTBR0_SIZE (PAGE_SIZE) > > +#else > > +#define RESERVED_TTBR0_SIZE (0) > > +#endif > > I was going to suggest that we use the empty_zero_page, which we can > address with an adrp, because I had forgotten that we need to generate > the *physical* address. > > It would be good if we could have a description of why we need the new > reserved page somewhere in the code. I'm sure I won't be the only one > tripped up by this. > > It would be possible to use the existing empty_zero_page, if we're happy > to have a MOVZ; MOVK; MOVK; MOVK sequence that we patch at boot-time. > That could be faster than an MRS on some implementations. I was trying to keep the number of instructions to a minimum in preference to potentially slightly faster sequence (I haven't done any benchmarks). On ARMv8.1+ implementations, we just end up with more nops. We could also do an ldr from a PC-relative address, it's one instruction and it may not be (significantly) slower than MRS + ADD. > > +static inline void uaccess_ttbr0_enable(void) > > +{ > > + unsigned long flags; > > + > > + /* > > + * Disable interrupts to avoid preemption and potential saved > > + * TTBR0_EL1 updates between reading the variable and the MSR. > > + */ > > + local_irq_save(flags); > > + write_sysreg(current_thread_info()->ttbr0, ttbr0_el1); > > + isb(); > > + local_irq_restore(flags); > > +} > > I don't follow what problem this actually protects us against. In the > case of preemption everything should be saved+restored transparently, or > things would go wrong as soon as we enable IRQs anyway. > > Is this a hold-over from a percpu approach rather than the > current_thread_info() approach? If we get preempted between reading current_thread_info()->ttbr0 and writing TTBR0_EL1, a series of context switches could lead to the update of the ASID part of ttbr0. The actual MSR would store an old ASID in TTBR0_EL1. > > +#else > > +static inline void uaccess_ttbr0_disable(void) > > +{ > > +} > > + > > +static inline void uaccess_ttbr0_enable(void) > > +{ > > +} > > +#endif > > I think that it's better to drop the ifdef and add: > > if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN)) > return; > > ... at the start of each function. GCC should optimize the entire thing > away when not used, but we'll get compiler coverage regardless, and > therefore less breakage. All the symbols we required should exist > regardless. The reason for this is that thread_info.ttbr0 is conditionally defined. I don't think the compiler would ignore it. > > .macro uaccess_enable, tmp1, tmp2 > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > +alternative_if_not ARM64_HAS_PAN > > + save_and_disable_irq \tmp2 // avoid preemption > > + uaccess_ttbr0_enable \tmp1 > > + restore_irq \tmp2 > > +alternative_else > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > + nop > > +alternative_endif > > +#endif > > How about something like: > > .macro alternative_endif_else_nop > alternative_else > .rept ((662b-661b) / 4) > nop > .endr > alternative_endif > .endm > > So for the above we could have: > > alternative_if_not ARM64_HAS_PAN > save_and_disable_irq \tmp2 > uaccess_ttbr0_enable \tmp1 > restore_irq \tmp2 > alternative_endif_else_nop > > I'll see about spinning a patch, or discovering why that happens to be > broken. This looks better. Minor comment, I would actually name the ending statement alternative_else_nop_endif to match the order in which you'd normally write them. > > * tables again to remove any speculatively loaded cache lines. > > */ > > mov x0, x25 > > - add x1, x26, #SWAPPER_DIR_SIZE > > + add x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE > > dmb sy > > bl __inval_cache_range > > > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > > index 659963d40bb4..fe393ccf9352 100644 > > --- a/arch/arm64/kernel/vmlinux.lds.S > > +++ b/arch/arm64/kernel/vmlinux.lds.S > > @@ -196,6 +196,11 @@ SECTIONS > > swapper_pg_dir = .; > > . += SWAPPER_DIR_SIZE; > > > > +#ifdef CONFIG_ARM64_TTBR0_PAN > > + reserved_ttbr0 = .; > > + . += PAGE_SIZE; > > +#endif > > Surely RESERVED_TTBR0_SIZE, as elsewhere? I'll try to move it somewhere where it can be included in vmlinux.lds.S (I can probably include cpufeature.h directly). -- Catalin
next prev parent reply other threads:[~2016-09-06 10:27 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-05 15:38 ` Mark Rutland 2016-09-05 15:38 ` [kernel-hardening] " Mark Rutland 2016-09-12 14:52 ` Catalin Marinas 2016-09-12 14:52 ` [kernel-hardening] " Catalin Marinas 2016-09-12 15:09 ` Mark Rutland 2016-09-12 15:09 ` [kernel-hardening] " Mark Rutland 2016-09-12 16:26 ` Catalin Marinas 2016-09-12 16:26 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:02 ` [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-05 16:11 ` Mark Rutland 2016-09-05 16:11 ` [kernel-hardening] " Mark Rutland 2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] [PATCH v2 3/7] arm64: Introduce uaccess_{disable,enable} " Catalin Marinas 2016-09-05 17:20 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} " Mark Rutland 2016-09-05 17:20 ` [kernel-hardening] " Mark Rutland 2016-09-06 10:27 ` Catalin Marinas [this message] 2016-09-06 10:27 ` Catalin Marinas 2016-09-06 10:45 ` Mark Rutland 2016-09-06 10:45 ` [kernel-hardening] " Mark Rutland 2016-09-11 13:55 ` Ard Biesheuvel 2016-09-11 13:55 ` Ard Biesheuvel 2016-09-12 9:32 ` Catalin Marinas 2016-09-12 9:32 ` Catalin Marinas 2016-09-09 17:15 ` Catalin Marinas 2016-09-09 17:15 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:02 ` [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-06 17:31 ` Mark Rutland 2016-09-06 17:31 ` [kernel-hardening] " Mark Rutland 2016-09-02 15:02 ` [PATCH v2 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:02 ` [PATCH v2 6/7] arm64: xen: Enable user access before a privcmd hvc call Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:02 ` [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN Catalin Marinas 2016-09-02 15:02 ` [kernel-hardening] " Catalin Marinas 2016-09-02 15:47 ` Mark Rutland 2016-09-02 15:47 ` [kernel-hardening] " Mark Rutland 2016-09-07 23:20 ` [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Kees Cook 2016-09-07 23:20 ` [kernel-hardening] " Kees Cook 2016-09-08 12:51 ` Catalin Marinas 2016-09-08 12:51 ` [kernel-hardening] " Catalin Marinas 2016-09-08 15:50 ` Kees Cook 2016-09-08 15:50 ` [kernel-hardening] " Kees Cook 2016-09-09 16:31 ` Mark Rutland 2016-09-09 16:31 ` [kernel-hardening] " Mark Rutland 2016-09-09 18:24 ` Kees Cook 2016-09-09 18:24 ` [kernel-hardening] " Kees Cook 2016-09-09 23:40 ` [kernel-hardening] " David Brown 2016-09-09 23:40 ` David Brown 2016-09-10 9:51 ` Catalin Marinas 2016-09-10 9:51 ` [kernel-hardening] " Catalin Marinas 2016-09-10 10:56 ` Ard Biesheuvel 2016-09-10 10:56 ` [kernel-hardening] " Ard Biesheuvel 2016-09-11 12:16 ` Catalin Marinas 2016-09-11 12:16 ` [kernel-hardening] " Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160906102741.GF19605@e104818-lin.cambridge.arm.com \ --to=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.