| Message ID | 20190529190332.29753-1-kristina.martsenko@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: return address signing | expand |
On Wed, May 29, 2019 at 08:03:25PM +0100, Kristina Martsenko wrote: > This series improves function return address protection for the arm64 kernel, by > compiling the kernel with ARMv8.3 Pointer Authentication instructions. This > should help protect the kernel against attacks using return-oriented > programming. Can you speak to whether this feature should be enalbed in addition to or instead of the standard stack canary option? > - The patches make use of the sign-return-address/branch-protection compiler > options and function attributes. GCC supports both, but Clang/LLVM appears > to only support the compiler option, not the function attribute, so with > these patches (and CONFIG_ARM64_PTR_AUTH=y) an LLVM-built kernel will fail > to boot on ARMv8.3 CPUs. I don't yet know why LLVM does not support it, or > whether support can be added. This series may need to be rewritten to not > use the attribute, and instead move the functionality to assembly, or to > disable return address signing when building with LLVM. I've added Luke Cheeseman and Diogo N. Sampaio to CC. In looking quickly at the LLVM support for branch-protection, I think it's just missing the attribute target glue needed to "notice" the attribute markings. Luke, is this expected to work Clang currently? I'm not familiar yet with how attributes get wired up, but I think it should be quite possible. > - more testing Is PAC emulated in QEmu yet? (I assume I can't get real hardware to help test this yet...)
On Wed, May 29, 2019 at 08:09:23PM -0700, Kees Cook wrote: > On Wed, May 29, 2019 at 08:03:25PM +0100, Kristina Martsenko wrote: > > This series improves function return address protection for the arm64 kernel, by > > compiling the kernel with ARMv8.3 Pointer Authentication instructions. This > > should help protect the kernel against attacks using return-oriented > > programming. > > Can you speak to whether this feature should be enalbed in addition to > or instead of the standard stack canary option? Hmm. That's a really interesting question. Given that PAC is optional in the hardware and behaves as NOPs on older CPUs, I've have thought that we'd need to continue enabling stack canaries regardless. However, that then raises the obvious question as to whether we could patch out the canary code if we detect PAC at runtime, which probably needs compiler help... Then again, perhaps there's benefit in having both features enabled. So I think I agree with your question :) Will
On Thu, 30 May 2019 at 09:25, Will Deacon <will.deacon@arm.com> wrote: > > On Wed, May 29, 2019 at 08:09:23PM -0700, Kees Cook wrote: > > On Wed, May 29, 2019 at 08:03:25PM +0100, Kristina Martsenko wrote: > > > This series improves function return address protection for the arm64 kernel, by > > > compiling the kernel with ARMv8.3 Pointer Authentication instructions. This > > > should help protect the kernel against attacks using return-oriented > > > programming. > > > > Can you speak to whether this feature should be enalbed in addition to > > or instead of the standard stack canary option? > > Hmm. That's a really interesting question. Given that PAC is optional in the > hardware and behaves as NOPs on older CPUs, I've have thought that we'd need > to continue enabling stack canaries regardless. However, that then raises > the obvious question as to whether we could patch out the canary code if we > detect PAC at runtime, which probably needs compiler help... > > Then again, perhaps there's benefit in having both features enabled. > > So I think I agree with your question :) > PAC only protects the return address, which is arguably the most vulnerable piece of data in the stack frame, but not the only one. So one question is whether we should take the hit of protecting ourselves from stack buffer overrun exploits that manage to overwrite something else in the stack frame while leaving the return address alone. I don't think doing that adds any value. Or if you are paranoid, you could argue that the stack protector also protects against a leak of the PAC key. And obviously, you need hardware from the future to use PAC :-) So while we think we should retain it for the single kernel image, I don't see why you would enable the stack protector on, e.g., android images built specifically for SOCs that enable PAC in the kernel.
On 30/05/2019 09:39, Ard Biesheuvel wrote: > On Thu, 30 May 2019 at 09:25, Will Deacon <will.deacon@arm.com> wrote: >> >> On Wed, May 29, 2019 at 08:09:23PM -0700, Kees Cook wrote: >> > On Wed, May 29, 2019 at 08:03:25PM +0100, Kristina Martsenko wrote: >> > > This series improves function return address protection for the arm64 kernel, by >> > > compiling the kernel with ARMv8.3 Pointer Authentication instructions. This >> > > should help protect the kernel against attacks using return-oriented >> > > programming. >> > >> > Can you speak to whether this feature should be enalbed in addition to >> > or instead of the standard stack canary option? >> >> Hmm. That's a really interesting question. Given that PAC is optional in the >> hardware and behaves as NOPs on older CPUs, I've have thought that we'd need >> to continue enabling stack canaries regardless. However, that then raises >> the obvious question as to whether we could patch out the canary code if we >> detect PAC at runtime, which probably needs compiler help... >> >> Then again, perhaps there's benefit in having both features enabled. >> >> So I think I agree with your question :) >> > > PAC only protects the return address, which is arguably the most > vulnerable piece of data in the stack frame, but not the only one. So > one question is whether we should take the hit of protecting ourselves > from stack buffer overrun exploits that manage to overwrite something > else in the stack frame while leaving the return address alone. I > don't think doing that adds any value. Or if you are paranoid, you > could argue that the stack protector also protects against a leak of > the PAC key. And obviously, you need hardware from the future to use > PAC :-) > > So while we think we should retain it for the single kernel image, I > don't see why you would enable the stack protector on, e.g., android > images built specifically for SOCs that enable PAC in the kernel. Return address signing using PAC only protects the return address and reduces the surface area for ROP oriented attacks on hardware that has PAC instructions. For a single image the answer today I think would be to have both return address signing and stack Canaries. In a few years (decades ?) when everyone has 8.3 silicon, then maybe canaries can be turned off ? If someone has control over deployment of kernels - Return address signing only if you knew it's on 8.3+ hardware. - Stack canaries if only on pre-8.3 hardware Regards, Ramana
>> - more testing > > Is PAC emulated in QEmu yet? (I assume I can't get real hardware to help > test this yet...) AFAIK, yes qemu trunk should have this. Thanks, Ramana > > -- > Kees Cook
On Thu, May 30, 2019 at 10:33:33AM +0000, Luke Cheeseman wrote: > > Luke, is this expected to work Clang currently? > > > Do you mean something like the following to control signing of each function? > > > int __attribute__ ((target ("sign-return-address=all"))) foo(void) { > return 42; > } Well, yes, though, in this usage, the goal is to disable it for specific functions: int __attribute__((target("branch-protection=none"))) early_func(void) { /* set up branch protection registers */ } > Clang doesn't currently support any function attribute to control > function signing to this level of granularity. We haven't added it and > don't have a plan to do so at the moment. What's needed to accomplish this? It looks to be a blocker for getting PAC working on Android kernels. -Kees > > > Thanks, > > Luke > > > ________________________________ > From: Kees Cook <keescook@chromium.org> > Sent: 30 May 2019 04:09:23 > To: Kristina Martsenko > Cc: Luke Cheeseman; Diogo Sampaio; linux-arm-kernel@lists.infradead.org; Amit Kachhap; Ard Biesheuvel; Catalin Marinas; Dave P Martin; Mark Rutland; Ramana Radhakrishnan; Suzuki Poulose; Will Deacon > Subject: Re: [RFC v2 0/7] arm64: return address signing > > On Wed, May 29, 2019 at 08:03:25PM +0100, Kristina Martsenko wrote: > > This series improves function return address protection for the arm64 kernel, by > > compiling the kernel with ARMv8.3 Pointer Authentication instructions. This > > should help protect the kernel against attacks using return-oriented > > programming. > > Can you speak to whether this feature should be enalbed in addition to > or instead of the standard stack canary option? > > > - The patches make use of the sign-return-address/branch-protection compiler > > options and function attributes. GCC supports both, but Clang/LLVM appears > > to only support the compiler option, not the function attribute, so with > > these patches (and CONFIG_ARM64_PTR_AUTH=y) an LLVM-built kernel will fail > > to boot on ARMv8.3 CPUs. I don't yet know why LLVM does not support it, or > > whether support can be added. This series may need to be rewritten to not > > use the attribute, and instead move the functionality to assembly, or to > > disable return address signing when building with LLVM. > > I've added Luke Cheeseman and Diogo N. Sampaio to CC. In looking quickly > at the LLVM support for branch-protection, I think it's just missing the > attribute target glue needed to "notice" the attribute markings. Luke, > is this expected to work Clang currently? I'm not familiar yet with > how attributes get wired up, but I think it should be quite possible. > > > - more testing > > Is PAC emulated in QEmu yet? (I assume I can't get real hardware to help > test this yet...) > > -- > Kees Cook > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote: > The semantics of this attribute are straightforward enough but it > raises some questions. One question being why would I want to turn off > BTI (also controlled by this option) for one function in a file? Which > gets a bit odd. It's about leaving very early CPU startup functions in the kernel from getting marked up (since they are running before or during the PAC setup). > I don't know if the alternatives have been suggested but it's > possible to achieve the result you seem to be after (a function without > return address signing) in a couple of ways. First and foremost, > separating the function out into it's own file and compiling with > -mbranch-protection=none. Alternatively, writing the function in assembly > or perhaps even a naked function with inline assembly. Fair enough. :) Thanks for the clarification. Yeah, split on compilation unit could work. (In the future, though, having the attribute flexibility would be nice.) Kristina, would it be feasible to split these functions into a separate source file? (There doesn't seem to be a need to inline them, given they're not performance sensitive and only used once, etc?)
On Thu, May 30, 2019 at 11:05:15AM -0700, Kees Cook wrote: > On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote: > > The semantics of this attribute are straightforward enough but it > > raises some questions. One question being why would I want to turn off > > BTI (also controlled by this option) for one function in a file? Which > > gets a bit odd. > > It's about leaving very early CPU startup functions in the kernel from > getting marked up (since they are running before or during the PAC setup). > > > I don't know if the alternatives have been suggested but it's > > possible to achieve the result you seem to be after (a function without > > return address signing) in a couple of ways. First and foremost, > > separating the function out into it's own file and compiling with > > -mbranch-protection=none. Alternatively, writing the function in assembly > > or perhaps even a naked function with inline assembly. > > Fair enough. :) Thanks for the clarification. Yeah, split on compilation > unit could work. (In the future, though, having the attribute flexibility > would be nice.) > > Kristina, would it be feasible to split these functions into a separate > source file? (There doesn't seem to be a need to inline them, given > they're not performance sensitive and only used once, etc?) Right, and we could call it kernel.c Sarcasm aside, please fix this in the toolchain. Moving logically unrelated functions into one file just because the toolchain doesn't yet support this feature just messes up the codebase and removes the incentive to get this implemented properly. After all, you need something to do now that asm goto is out of the way, right? ;) Will
On Fri, May 31, 2019 at 10:22:02AM +0100, Will Deacon wrote: > On Thu, May 30, 2019 at 11:05:15AM -0700, Kees Cook wrote: > > On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote: > > > The semantics of this attribute are straightforward enough but it > > > raises some questions. One question being why would I want to turn off > > > BTI (also controlled by this option) for one function in a file? Which > > > gets a bit odd. > > > > It's about leaving very early CPU startup functions in the kernel from > > getting marked up (since they are running before or during the PAC setup). > > > > > I don't know if the alternatives have been suggested but it's > > > possible to achieve the result you seem to be after (a function without > > > return address signing) in a couple of ways. First and foremost, > > > separating the function out into it's own file and compiling with > > > -mbranch-protection=none. Alternatively, writing the function in assembly > > > or perhaps even a naked function with inline assembly. > > > > Fair enough. :) Thanks for the clarification. Yeah, split on compilation > > unit could work. (In the future, though, having the attribute flexibility > > would be nice.) > > > > Kristina, would it be feasible to split these functions into a separate > > source file? (There doesn't seem to be a need to inline them, given > > they're not performance sensitive and only used once, etc?) > > Right, and we could call it kernel.c > > Sarcasm aside, please fix this in the toolchain. Moving logically unrelated > functions into one file just because the toolchain doesn't yet support this > feature just messes up the codebase and removes the incentive to get this > implemented properly. After all, you need something to do now that asm goto > is out of the way, right? ;) LLVM tracking bug created... https://bugs.llvm.org/show_bug.cgi?id=42095
On Sun, Jun 02, 2019 at 08:43:55AM -0700, Kees Cook wrote: > On Fri, May 31, 2019 at 10:22:02AM +0100, Will Deacon wrote: > > On Thu, May 30, 2019 at 11:05:15AM -0700, Kees Cook wrote: > > > On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote: > > > > The semantics of this attribute are straightforward enough but it > > > > raises some questions. One question being why would I want to turn off > > > > BTI (also controlled by this option) for one function in a file? Which > > > > gets a bit odd. > > > > > > It's about leaving very early CPU startup functions in the kernel from > > > getting marked up (since they are running before or during the PAC setup). > > > > > > > I don't know if the alternatives have been suggested but it's > > > > possible to achieve the result you seem to be after (a function without > > > > return address signing) in a couple of ways. First and foremost, > > > > separating the function out into it's own file and compiling with > > > > -mbranch-protection=none. Alternatively, writing the function in assembly > > > > or perhaps even a naked function with inline assembly. > > > > > > Fair enough. :) Thanks for the clarification. Yeah, split on compilation > > > unit could work. (In the future, though, having the attribute flexibility > > > would be nice.) > > > > > > Kristina, would it be feasible to split these functions into a separate > > > source file? (There doesn't seem to be a need to inline them, given > > > they're not performance sensitive and only used once, etc?) > > > > Right, and we could call it kernel.c > > > > Sarcasm aside, please fix this in the toolchain. Moving logically unrelated > > functions into one file just because the toolchain doesn't yet support this > > feature just messes up the codebase and removes the incentive to get this > > implemented properly. After all, you need something to do now that asm goto > > is out of the way, right? ;) > > LLVM tracking bug created... > https://bugs.llvm.org/show_bug.cgi?id=42095 Thanks, Kees! Will
Hi,
It was suggested to me that forcing the compiler to inline the function may be another way to avoid writing the function in a separate file and not have the concerns of switching keys in a function. For example:
void __attribute__((always_inline)) switch_keys() {
// do something
}
int main() {
switch_keys(42);
}
switch_keys is always inlined so you don't get the pac/aut pair. Is this something that is useful?
For the feature request for disabling branch protection (https://bugs.llvm.org/show_bug.cgi?id=42095) is there a time frame you need this within?
Thanks,
Luke
From: Will Deacon <will.deacon@arm.com>
Sent: 03 June 2019 11:40
To: Kees Cook
Cc: Luke Cheeseman; Kristina Martsenko; Luke Cheeseman; Diogo Sampaio; linux-arm-kernel@lists.infradead.org; Amit Kachhap; Ard Biesheuvel; Catalin Marinas; Dave P Martin; Mark Rutland; Ramana Radhakrishnan; Suzuki Poulose; Kristof Beyls; Christof Douma
Subject: Re: [RFC v2 0/7] arm64: return address signing
On Sun, Jun 02, 2019 at 08:43:55AM -0700, Kees Cook wrote:
> On Fri, May 31, 2019 at 10:22:02AM +0100, Will Deacon wrote:
> > On Thu, May 30, 2019 at 11:05:15AM -0700, Kees Cook wrote:
> > > On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote:
> > > > The semantics of this attribute are straightforward enough but it
> > > > raises some questions. One question being why would I want to turn off
> > > > BTI (also controlled by this option) for one function in a file? Which
> > > > gets a bit odd.
> > >
> > > It's about leaving very early CPU startup functions in the kernel from
> > > getting marked up (since they are running before or during the PAC setup).
> > >
> > > > I don't know if the alternatives have been suggested but it's
> > > > possible to achieve the result you seem to be after (a function without
> > > > return address signing) in a couple of ways. First and foremost,
> > > > separating the function out into it's own file and compiling with
> > > > -mbranch-protection=none. Alternatively, writing the function in assembly
> > > > or perhaps even a naked function with inline assembly.
> > >
> > > Fair enough. :) Thanks for the clarification. Yeah, split on compilation
> > > unit could work. (In the future, though, having the attribute flexibility
> > > would be nice.)
> > >
> > > Kristina, would it be feasible to split these functions into a separate
> > > source file? (There doesn't seem to be a need to inline them, given
> > > they're not performance sensitive and only used once, etc?)
> >
> > Right, and we could call it kernel.c
> >
> > Sarcasm aside, please fix this in the toolchain. Moving logically unrelated
> > functions into one file just because the toolchain doesn't yet support this
> > feature just messes up the codebase and removes the incentive to get this
> > implemented properly. After all, you need something to do now that asm goto
> > is out of the way, right? ;)
>
> LLVM tracking bug created...
> https://bugs.llvm.org/show_bug.cgi?id=42095
Thanks, Kees!
Will
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On 04/06/2019 14:52, Luke Cheeseman wrote: > Hi, > > It was suggested to me that forcing the compiler to inline the function may be another way to avoid writing the function in a separate file and not have the concerns of switching keys in a function. For example: > > void __attribute__((always_inline)) switch_keys() { > // do something > } > > > int main() { > switch_keys(42); > } > > > switch_keys is always inlined so you don't get the pac/aut pair. Is this something that is useful? This is useful in some cases, but not when the function and its caller are in different compilation units. Unfortunately we have cases where arm64-specific code (that sets up the keys) is being called from core kernel code, which is in a different file. For this case we'd still need the attribute to disable return address signing. Thanks, Kristina > > For the feature request for disabling branch protection (https://bugs.llvm.org/show_bug.cgi?id=42095) is there a time frame you need this within? > > Thanks, > Luke > > > From: Will Deacon <will.deacon@arm.com> > Sent: 03 June 2019 11:40 > To: Kees Cook > Cc: Luke Cheeseman; Kristina Martsenko; Luke Cheeseman; Diogo Sampaio; linux-arm-kernel@lists.infradead.org; Amit Kachhap; Ard Biesheuvel; Catalin Marinas; Dave P Martin; Mark Rutland; Ramana Radhakrishnan; Suzuki Poulose; Kristof Beyls; Christof Douma > Subject: Re: [RFC v2 0/7] arm64: return address signing > > > On Sun, Jun 02, 2019 at 08:43:55AM -0700, Kees Cook wrote: >> On Fri, May 31, 2019 at 10:22:02AM +0100, Will Deacon wrote: >>> On Thu, May 30, 2019 at 11:05:15AM -0700, Kees Cook wrote: >>>> On Thu, May 30, 2019 at 04:55:08PM +0000, Luke Cheeseman wrote: >>>>> The semantics of this attribute are straightforward enough but it >>>>> raises some questions. One question being why would I want to turn off >>>>> BTI (also controlled by this option) for one function in a file? Which >>>>> gets a bit odd. >>>> >>>> It's about leaving very early CPU startup functions in the kernel from >>>> getting marked up (since they are running before or during the PAC setup). >>>> >>>>> I don't know if the alternatives have been suggested but it's >>>>> possible to achieve the result you seem to be after (a function without >>>>> return address signing) in a couple of ways. First and foremost, >>>>> separating the function out into it's own file and compiling with >>>>> -mbranch-protection=none. Alternatively, writing the function in assembly >>>>> or perhaps even a naked function with inline assembly. >>>> >>>> Fair enough. :) Thanks for the clarification. Yeah, split on compilation >>>> unit could work. (In the future, though, having the attribute flexibility >>>> would be nice.) >>>> >>>> Kristina, would it be feasible to split these functions into a separate >>>> source file? (There doesn't seem to be a need to inline them, given >>>> they're not performance sensitive and only used once, etc?) >>> >>> Right, and we could call it kernel.c >>> >>> Sarcasm aside, please fix this in the toolchain. Moving logically unrelated >>> functions into one file just because the toolchain doesn't yet support this >>> feature just messes up the codebase and removes the incentive to get this >>> implemented properly. After all, you need something to do now that asm goto >>> is out of the way, right? ;) >> >> LLVM tracking bug created... >> https://bugs.llvm.org/show_bug.cgi?id=42095 > > Thanks, Kees! > > Will > >
On 30/05/2019 10:12, Ramana Radhakrishnan wrote: > >>> - more testing >> >> Is PAC emulated in QEmu yet? (I assume I can't get real hardware to help >> test this yet...) > > AFAIK, yes qemu trunk should have this. I've been testing on the ARM FastModels, but I tried out QEMU 4.0.0 and it seems to support PAC with the "-cpu max" option. Thanks, Kristina
On Thu, Jun 06, 2019 at 06:44:41PM +0100, Kristina Martsenko wrote: > On 30/05/2019 10:12, Ramana Radhakrishnan wrote: > > > >>> - more testing > >> > >> Is PAC emulated in QEmu yet? (I assume I can't get real hardware to help > >> test this yet...) > > > > AFAIK, yes qemu trunk should have this. > > I've been testing on the ARM FastModels, but I tried out QEMU 4.0.0 and > it seems to support PAC with the "-cpu max" option. Ah-ha! I wasn't seeing it mentioned in dmesg (it should appear along with PAN, etc, yes?) but I guess I need a newer QEMU: $ qemu-system-aarch64 --version QEMU emulator version 3.1.0 (Debian 1:3.1+dfsg-2ubuntu3.1) I will go build QEMU myself! :)