| Message ID | 20220901154127.2120577-1-scott@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: Work around missing `bti c` in modules | expand |
D Scott Phillips <scott@os.amperecomputing.com> writes: > GCC does not insert a `bti c` instruction at the beginning of a function > when all callers reach the function through a direct branch[1]. In the case > of cross-section calls (like __init to non __init), a thunk may be inserted > which uses an indirect branch. If that happens, the first instruction in > the callee function will result in a Branch Target Exception due to the > missing `bti c`. > > Handle Branch Target Exceptions which happen in the kernel due to module > calls from __init to non-__init by clearing PSTATE.BTYPE and resuming. > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106671 > > Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com> > --- > Changes since v1: > - Add the gcc bug id into the traps.c comment > - Cover the try_module_get with the preempt_disable > - Add a CC_HAS_ config for the compiler bug that we'll eventually refine > > arch/arm64/Kconfig | 3 +++ > arch/arm64/kernel/entry-common.c | 12 +++++++++ > arch/arm64/kernel/traps.c | 43 ++++++++++++++++++++++++++++++-- > 3 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index cd93c9041679..d5d4d2891657 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1860,6 +1860,9 @@ config ARM64_BTI_KERNEL > is enabled and the system supports BTI all kernel code including > modular code must have BTI enabled. > > +config CC_HAS_CROSS_SECTION_BTI_MISSING > + def_bool CC_IS_GCC # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106671 > + > config CC_HAS_BRANCH_PROT_PAC_RET_BTI > # GCC 9 or later, clang 8 or later > def_bool $(cc-option,-mbranch-protection=pac-ret+leaf+bti) > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 56cefd33eb8e..696e3f3c90ea 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -388,6 +388,15 @@ static void noinstr el1_undef(struct pt_regs *regs) > exit_to_kernel_mode(regs); > } > > +static void noinstr el1_bti(struct pt_regs *regs) > +{ > + enter_from_kernel_mode(regs); > + local_daif_inherit(regs); > + do_bti(regs); > + local_daif_mask(); > + exit_to_kernel_mode(regs); > +} > + > static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr) > { > unsigned long far = read_sysreg(far_el1); > @@ -427,6 +436,9 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs) > case ESR_ELx_EC_UNKNOWN: > el1_undef(regs); > break; > + case ESR_ELx_EC_BTI: > + el1_bti(regs); > + break; > case ESR_ELx_EC_BREAKPT_CUR: > case ESR_ELx_EC_SOFTSTP_CUR: > case ESR_ELx_EC_WATCHPT_CUR: There's a change in behavior here that I don't want to go by unnoticed. Previously BTI exceptions would fall through to the default case and cause a panic. With this change they'll go into do_bti, and then kill the task if not handled by the gcc workaround case. I think that change is a good one, but I don't want to sneak it in. Would it be better if I split that out into a separate patch so that it gets noticed on its own? Scott
On Thu, Sep 01, 2022 at 08:52:27AM -0700, D Scott Phillips wrote: > There's a change in behavior here that I don't want to go by > unnoticed. Previously BTI exceptions would fall through to the default > case and cause a panic. With this change they'll go into do_bti, and > then kill the task if not handled by the gcc workaround case. I think > that change is a good one, but I don't want to sneak it in. > Would it be better if I split that out into a separate patch so that it > gets noticed on its own? I think so. It should at least be highlighted in the commit log. Personally I don't have a strong opinion on which behaviour we go with, there's arguments either way.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index cd93c9041679..d5d4d2891657 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1860,6 +1860,9 @@ config ARM64_BTI_KERNEL is enabled and the system supports BTI all kernel code including modular code must have BTI enabled. +config CC_HAS_CROSS_SECTION_BTI_MISSING + def_bool CC_IS_GCC # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106671 + config CC_HAS_BRANCH_PROT_PAC_RET_BTI # GCC 9 or later, clang 8 or later def_bool $(cc-option,-mbranch-protection=pac-ret+leaf+bti) diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index 56cefd33eb8e..696e3f3c90ea 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -388,6 +388,15 @@ static void noinstr el1_undef(struct pt_regs *regs) exit_to_kernel_mode(regs); } +static void noinstr el1_bti(struct pt_regs *regs) +{ + enter_from_kernel_mode(regs); + local_daif_inherit(regs); + do_bti(regs); + local_daif_mask(); + exit_to_kernel_mode(regs); +} + static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr) { unsigned long far = read_sysreg(far_el1); @@ -427,6 +436,9 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs) case ESR_ELx_EC_UNKNOWN: el1_undef(regs); break; + case ESR_ELx_EC_BTI: + el1_bti(regs); + break; case ESR_ELx_EC_BREAKPT_CUR: case ESR_ELx_EC_SOFTSTP_CUR: case ESR_ELx_EC_WATCHPT_CUR: diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 9ac7a81b79be..f1135166ecdb 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -501,8 +501,47 @@ NOKPROBE_SYMBOL(do_undefinstr); void do_bti(struct pt_regs *regs) { - BUG_ON(!user_mode(regs)); - force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0); + struct module *mod; + + if (user_mode(regs)) { + force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0); + return; + } + + /* + * GCC does not insert a `bti c` instruction at the beginning + * of a function when all callers reach the function through a + * direct branch. In the case of cross-section calls (like + * __init to non __init), a thunk may be inserted which uses + * an indirect branch. If that happens, the first instruction + * in the callee function will result in a Branch Target + * Exception due to the missing `bti c`. + * + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106671 + * + * If that's the case here, clear PSTATE.BTYPE and resume. + */ + if (IS_ENABLED(CONFIG_CC_HAS_CROSS_SECTION_BTI_MISSING)) { + preempt_disable(); + mod = __module_text_address(regs->pc); + + if (mod && try_module_get(mod)) { + bool from_init; + + from_init = within_module_init(regs->regs[30], mod); + module_put(mod); + + if (from_init) { + preempt_enable(); + regs->pstate &= ~PSR_BTYPE_MASK; + return; + } + } + + preempt_enable(); + } + + die("Oops - BTI", regs, 0); } NOKPROBE_SYMBOL(do_bti);
GCC does not insert a `bti c` instruction at the beginning of a function when all callers reach the function through a direct branch[1]. In the case of cross-section calls (like __init to non __init), a thunk may be inserted which uses an indirect branch. If that happens, the first instruction in the callee function will result in a Branch Target Exception due to the missing `bti c`. Handle Branch Target Exceptions which happen in the kernel due to module calls from __init to non-__init by clearing PSTATE.BTYPE and resuming. [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106671 Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com> --- Changes since v1: - Add the gcc bug id into the traps.c comment - Cover the try_module_get with the preempt_disable - Add a CC_HAS_ config for the compiler bug that we'll eventually refine arch/arm64/Kconfig | 3 +++ arch/arm64/kernel/entry-common.c | 12 +++++++++ arch/arm64/kernel/traps.c | 43 ++++++++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 2 deletions(-)