| Message ID | 20210914191045.2234020-11-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Add support for Clang CFI | expand |
On Tue, Sep 14, 2021 at 12:10:39PM -0700, Sami Tolvanen wrote: > Exception tables are populated in assembly code, but the handlers are > called in fixup_exception, which trips indirect call checking with > CONFIG_CFI_CLANG. Mark the handlers __cficanonical to allow addresses > taken in assembly to pass CFI checking. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > --- > arch/x86/mm/extable.c | 64 ++++++++++++++++++++++++------------------- > 1 file changed, 36 insertions(+), 28 deletions(-) > > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > index e1664e9f969c..d16912dcbb4e 100644 > --- a/arch/x86/mm/extable.c > +++ b/arch/x86/mm/extable.c > @@ -24,16 +24,18 @@ ex_fixup_handler(const struct exception_table_entry *x) > return (ex_handler_t)((unsigned long)&x->handler + x->handler); > } > > -__visible bool ex_handler_default(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_default(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > regs->ip = ex_fixup_addr(fixup); > return true; > } > EXPORT_SYMBOL(ex_handler_default); > > +__visible __cficanonical > __visible bool ex_handler_fault(const struct exception_table_entry *fixup, Double __visible here, but with that fixed: Reviewed-by: Kees Cook <keescook@chromium.org> I would note that given Linus's recent comments on attribute locations, it does seem that __cficanonical is more a function behavior attribute than a storage class... I'm not really sure: https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com -Kees > struct pt_regs *regs, int trapnr, > unsigned long error_code, > @@ -55,10 +57,11 @@ EXPORT_SYMBOL_GPL(ex_handler_fault); > * of vulnerability by restoring from the initial state (essentially, zeroing > * out all the FPU registers) if we can't restore from the task's FPU state. > */ > -__visible bool ex_handler_fprestore(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_fprestore(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > regs->ip = ex_fixup_addr(fixup); > > @@ -70,10 +73,11 @@ __visible bool ex_handler_fprestore(const struct exception_table_entry *fixup, > } > EXPORT_SYMBOL_GPL(ex_handler_fprestore); > > -__visible bool ex_handler_uaccess(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_uaccess(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?"); > regs->ip = ex_fixup_addr(fixup); > @@ -81,10 +85,11 @@ __visible bool ex_handler_uaccess(const struct exception_table_entry *fixup, > } > EXPORT_SYMBOL(ex_handler_uaccess); > > -__visible bool ex_handler_copy(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_copy(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?"); > regs->ip = ex_fixup_addr(fixup); > @@ -93,10 +98,11 @@ __visible bool ex_handler_copy(const struct exception_table_entry *fixup, > } > EXPORT_SYMBOL(ex_handler_copy); > > -__visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > if (pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", > (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) > @@ -110,10 +116,11 @@ __visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup > } > EXPORT_SYMBOL(ex_handler_rdmsr_unsafe); > > -__visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > if (pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", > (unsigned int)regs->cx, (unsigned int)regs->dx, > @@ -126,10 +133,11 @@ __visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup > } > EXPORT_SYMBOL(ex_handler_wrmsr_unsafe); > > -__visible bool ex_handler_clear_fs(const struct exception_table_entry *fixup, > - struct pt_regs *regs, int trapnr, > - unsigned long error_code, > - unsigned long fault_addr) > +__visible __cficanonical > +bool ex_handler_clear_fs(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr, > + unsigned long error_code, > + unsigned long fault_addr) > { > if (static_cpu_has(X86_BUG_NULL_SEG)) > asm volatile ("mov %0, %%fs" : : "rm" (__USER_DS)); > -- > 2.33.0.309.g3052b89438-goog >
On Tue, Sep 14, 2021 at 12:37 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Sep 14, 2021 at 12:10:39PM -0700, Sami Tolvanen wrote: > > Exception tables are populated in assembly code, but the handlers are > > called in fixup_exception, which trips indirect call checking with > > CONFIG_CFI_CLANG. Mark the handlers __cficanonical to allow addresses > > taken in assembly to pass CFI checking. > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > --- > > arch/x86/mm/extable.c | 64 ++++++++++++++++++++++++------------------- > > 1 file changed, 36 insertions(+), 28 deletions(-) > > > > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > > index e1664e9f969c..d16912dcbb4e 100644 > > --- a/arch/x86/mm/extable.c > > +++ b/arch/x86/mm/extable.c > > @@ -24,16 +24,18 @@ ex_fixup_handler(const struct exception_table_entry *x) > > return (ex_handler_t)((unsigned long)&x->handler + x->handler); > > } > > > > -__visible bool ex_handler_default(const struct exception_table_entry *fixup, > > - struct pt_regs *regs, int trapnr, > > - unsigned long error_code, > > - unsigned long fault_addr) > > +__visible __cficanonical > > +bool ex_handler_default(const struct exception_table_entry *fixup, > > + struct pt_regs *regs, int trapnr, > > + unsigned long error_code, > > + unsigned long fault_addr) > > { > > regs->ip = ex_fixup_addr(fixup); > > return true; > > } > > EXPORT_SYMBOL(ex_handler_default); > > > > +__visible __cficanonical > > __visible bool ex_handler_fault(const struct exception_table_entry *fixup, > > Double __visible here, but with that fixed: Ah, thanks for noticing that. I'll fix it in the next version. > > Reviewed-by: Kees Cook <keescook@chromium.org> > > I would note that given Linus's recent comments on attribute locations, > it does seem that __cficanonical is more a function behavior attribute > than a storage class... I'm not really sure: > https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com __visible is not really a storage class either, but I thought it would make sense to keep these attributes together. I can certainly move them if anyone has strong feelings about the location. Sami
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index e1664e9f969c..d16912dcbb4e 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -24,16 +24,18 @@ ex_fixup_handler(const struct exception_table_entry *x) return (ex_handler_t)((unsigned long)&x->handler + x->handler); } -__visible bool ex_handler_default(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_default(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { regs->ip = ex_fixup_addr(fixup); return true; } EXPORT_SYMBOL(ex_handler_default); +__visible __cficanonical __visible bool ex_handler_fault(const struct exception_table_entry *fixup, struct pt_regs *regs, int trapnr, unsigned long error_code, @@ -55,10 +57,11 @@ EXPORT_SYMBOL_GPL(ex_handler_fault); * of vulnerability by restoring from the initial state (essentially, zeroing * out all the FPU registers) if we can't restore from the task's FPU state. */ -__visible bool ex_handler_fprestore(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_fprestore(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { regs->ip = ex_fixup_addr(fixup); @@ -70,10 +73,11 @@ __visible bool ex_handler_fprestore(const struct exception_table_entry *fixup, } EXPORT_SYMBOL_GPL(ex_handler_fprestore); -__visible bool ex_handler_uaccess(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_uaccess(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?"); regs->ip = ex_fixup_addr(fixup); @@ -81,10 +85,11 @@ __visible bool ex_handler_uaccess(const struct exception_table_entry *fixup, } EXPORT_SYMBOL(ex_handler_uaccess); -__visible bool ex_handler_copy(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_copy(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?"); regs->ip = ex_fixup_addr(fixup); @@ -93,10 +98,11 @@ __visible bool ex_handler_copy(const struct exception_table_entry *fixup, } EXPORT_SYMBOL(ex_handler_copy); -__visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { if (pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n", (unsigned int)regs->cx, regs->ip, (void *)regs->ip)) @@ -110,10 +116,11 @@ __visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup } EXPORT_SYMBOL(ex_handler_rdmsr_unsafe); -__visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { if (pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n", (unsigned int)regs->cx, (unsigned int)regs->dx, @@ -126,10 +133,11 @@ __visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup } EXPORT_SYMBOL(ex_handler_wrmsr_unsafe); -__visible bool ex_handler_clear_fs(const struct exception_table_entry *fixup, - struct pt_regs *regs, int trapnr, - unsigned long error_code, - unsigned long fault_addr) +__visible __cficanonical +bool ex_handler_clear_fs(const struct exception_table_entry *fixup, + struct pt_regs *regs, int trapnr, + unsigned long error_code, + unsigned long fault_addr) { if (static_cpu_has(X86_BUG_NULL_SEG)) asm volatile ("mov %0, %%fs" : : "rm" (__USER_DS));
Exception tables are populated in assembly code, but the handlers are called in fixup_exception, which trips indirect call checking with CONFIG_CFI_CLANG. Mark the handlers __cficanonical to allow addresses taken in assembly to pass CFI checking. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- arch/x86/mm/extable.c | 64 ++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 28 deletions(-)