| Message ID | 20200512092155.56931-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: bti: Fix support for userspace only BTI | expand |
On Tue, May 12, 2020 at 10:21:55AM +0100, Mark Brown wrote: > When setting PTE_MAYBE_GP we check system_supports_bti() but this is > true for systems where only CONFIG_BTI is set causing us to enable BTI > on some kernel text. Add an extra check for the kernel mode option, > using an ifdef due to line length. > > Fixes: c8027285e366 (arm64: Set GP bit in kernel page tables to enable BTI for the kernel) > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/pgtable-prot.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h > index 310690332896..3f07a044bdfb 100644 > --- a/arch/arm64/include/asm/pgtable-prot.h > +++ b/arch/arm64/include/asm/pgtable-prot.h > @@ -32,7 +32,11 @@ extern bool arm64_use_ng_mappings; > #define PTE_MAYBE_NG (arm64_use_ng_mappings ? PTE_NG : 0) > #define PMD_MAYBE_NG (arm64_use_ng_mappings ? PMD_SECT_NG : 0) > Might be worth a comment here in case somebody tries to "clean this up" later on! > +#ifdef CONFIG_ARM64_BTI_KERNEL > #define PTE_MAYBE_GP (system_supports_bti() ? PTE_GP : 0) > +#else > +#define PTE_MAYBE_GP 0 > +#endif I also notice we have a funny check in the vdso code: if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) && system_supports_bti()) but why do we care about CONFIG_ARM64_BTI_KERNEL for the vdso pages? Will
On Tue, May 12, 2020 at 11:39:09AM +0100, Will Deacon wrote: > Might be worth a comment here in case somebody tries to "clean this up" > later on! > > +#ifdef CONFIG_ARM64_BTI_KERNEL > > #define PTE_MAYBE_GP (system_supports_bti() ? PTE_GP : 0) > > +#else > > +#define PTE_MAYBE_GP 0 > > +#endif Sure. > I also notice we have a funny check in the vdso code: > if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) && system_supports_bti()) > but why do we care about CONFIG_ARM64_BTI_KERNEL for the vdso pages? To enable BTI for the vDSO we need the vDSO to be built with BTI annotations. Currently the CFLAGS are the same for the vDSO and the kernel, we could arrange to allow them to differ but since the most likely reason why the user has userspace but not kernel BTI is that their compiler isn't suitable it seemed like disproportionate effort for a most likely small audience.
On Tue, May 12, 2020 at 12:05:02PM +0100, Mark Brown wrote: > On Tue, May 12, 2020 at 11:39:09AM +0100, Will Deacon wrote: > > > Might be worth a comment here in case somebody tries to "clean this up" > > later on! > > > > +#ifdef CONFIG_ARM64_BTI_KERNEL > > > #define PTE_MAYBE_GP (system_supports_bti() ? PTE_GP : 0) > > > +#else > > > +#define PTE_MAYBE_GP 0 > > > +#endif > > Sure. > > > I also notice we have a funny check in the vdso code: > > > if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) && system_supports_bti()) > > > but why do we care about CONFIG_ARM64_BTI_KERNEL for the vdso pages? > > To enable BTI for the vDSO we need the vDSO to be built with BTI > annotations. Currently the CFLAGS are the same for the vDSO and the > kernel, we could arrange to allow them to differ but since the most > likely reason why the user has userspace but not kernel BTI is that > their compiler isn't suitable it seemed like disproportionate effort > for a most likely small audience. I was thinking of using CC_HAS_BRANCH_PROT_PAC_RET_BTI instead, but I'd forgotten about that GCC bug. Oh well. Will
On Tue, May 12, 2020 at 12:05:02PM +0100, Mark Brown wrote: > On Tue, May 12, 2020 at 11:39:09AM +0100, Will Deacon wrote: > > > Might be worth a comment here in case somebody tries to "clean this up" > > later on! > > > > +#ifdef CONFIG_ARM64_BTI_KERNEL > > > #define PTE_MAYBE_GP (system_supports_bti() ? PTE_GP : 0) > > > +#else > > > +#define PTE_MAYBE_GP 0 > > > +#endif > > Sure. > > > I also notice we have a funny check in the vdso code: > > > if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) && system_supports_bti()) > > > but why do we care about CONFIG_ARM64_BTI_KERNEL for the vdso pages? > > To enable BTI for the vDSO we need the vDSO to be built with BTI > annotations. Currently the CFLAGS are the same for the vDSO and the > kernel, we could arrange to allow them to differ but since the most > likely reason why the user has userspace but not kernel BTI is that > their compiler isn't suitable it seemed like disproportionate effort > for a most likely small audience. The situation where the user is stuck on a binary vendor kernel built using obsolete or wrongly configured tools doesn't sound that unlikely to me. When the vdso was all asm, we could easily have solved this by manually annotating it ... I guess that's the price of progress, unless anyone has a bright idea :/ Cheers ---Dave
On Wed, May 13, 2020 at 03:46:25PM +0100, Dave Martin wrote: > On Tue, May 12, 2020 at 12:05:02PM +0100, Mark Brown wrote: > > To enable BTI for the vDSO we need the vDSO to be built with BTI > > annotations. Currently the CFLAGS are the same for the vDSO and the > > kernel, we could arrange to allow them to differ but since the most > > likely reason why the user has userspace but not kernel BTI is that > > their compiler isn't suitable it seemed like disproportionate effort > > for a most likely small audience. > The situation where the user is stuck on a binary vendor kernel built > using obsolete or wrongly configured tools doesn't sound that unlikely > to me. They'd need to be stuck on a binary vendor kernel which is sufficiently new to have BTI support but using toolchains that are sufficiently old to be unable to BTI the kernel (if their vendor went out of their way to disable BTI then that's another issue). My guess is that people will tend to update major versions of these at relatively similar times, and hopefully if people are going to the trouble of backporting BTI they'll also grab the toolchain bits since it's substantially more useful if you can compile userspace. > When the vdso was all asm, we could easily have solved this by manually > annotating it ... I guess that's the price of progress, unless anyone > has a bright idea :/ Which is the other bit - if we don't trust the toolchain to build the kernel then we probably ought to have concerns about the C bits of the vDSO as well. Even if they're fine now some future vDSO change could cause this to explode.
On Wed, May 13, 2020 at 04:08:22PM +0100, Mark Brown wrote: > On Wed, May 13, 2020 at 03:46:25PM +0100, Dave Martin wrote: > > On Tue, May 12, 2020 at 12:05:02PM +0100, Mark Brown wrote: > > > > To enable BTI for the vDSO we need the vDSO to be built with BTI > > > annotations. Currently the CFLAGS are the same for the vDSO and the > > > kernel, we could arrange to allow them to differ but since the most > > > likely reason why the user has userspace but not kernel BTI is that > > > their compiler isn't suitable it seemed like disproportionate effort > > > for a most likely small audience. > > > The situation where the user is stuck on a binary vendor kernel built > > using obsolete or wrongly configured tools doesn't sound that unlikely > > to me. > > They'd need to be stuck on a binary vendor kernel which is sufficiently > new to have BTI support but using toolchains that are sufficiently old > to be unable to BTI the kernel (if their vendor went out of their way to > disable BTI then that's another issue). My guess is that people will > tend to update major versions of these at relatively similar times, and > hopefully if people are going to the trouble of backporting BTI they'll > also grab the toolchain bits since it's substantially more useful if you > can compile userspace. Yeah, just playing devil's advocate. > > When the vdso was all asm, we could easily have solved this by manually > > annotating it ... I guess that's the price of progress, unless anyone > > has a bright idea :/ > > Which is the other bit - if we don't trust the toolchain to build the > kernel then we probably ought to have concerns about the C bits of the > vDSO as well. Even if they're fine now some future vDSO change could > cause this to explode. Exactly. So I guess you're right: no kernel BTI, no vDSO BTI. It's a bit annoying, but there it is. I can't argue that the best fix for broken tools isn't to fix the tools ;) Cheers ---Dave
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h index 310690332896..3f07a044bdfb 100644 --- a/arch/arm64/include/asm/pgtable-prot.h +++ b/arch/arm64/include/asm/pgtable-prot.h @@ -32,7 +32,11 @@ extern bool arm64_use_ng_mappings; #define PTE_MAYBE_NG (arm64_use_ng_mappings ? PTE_NG : 0) #define PMD_MAYBE_NG (arm64_use_ng_mappings ? PMD_SECT_NG : 0) +#ifdef CONFIG_ARM64_BTI_KERNEL #define PTE_MAYBE_GP (system_supports_bti() ? PTE_GP : 0) +#else +#define PTE_MAYBE_GP 0 +#endif #define PROT_DEFAULT (_PROT_DEFAULT | PTE_MAYBE_NG) #define PROT_SECT_DEFAULT (_PROT_SECT_DEFAULT | PMD_MAYBE_NG)
When setting PTE_MAYBE_GP we check system_supports_bti() but this is true for systems where only CONFIG_BTI is set causing us to enable BTI on some kernel text. Add an extra check for the kernel mode option, using an ifdef due to line length. Fixes: c8027285e366 (arm64: Set GP bit in kernel page tables to enable BTI for the kernel) Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/pgtable-prot.h | 4 ++++ 1 file changed, 4 insertions(+)