| Message ID | 20210330205750.428816-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Optionally randomize kernel stack offset each syscall | expand |
On Tue, Mar 30 2021 at 13:57, Kees Cook wrote: > +/* > + * Do not use this anywhere else in the kernel. This is used here because > + * it provides an arch-agnostic way to grow the stack with correct > + * alignment. Also, since this use is being explicitly masked to a max of > + * 10 bits, stack-clash style attacks are unlikely. For more details see > + * "VLAs" in Documentation/process/deprecated.rst > + * The asm statement is designed to convince the compiler to keep the > + * allocation around even after "ptr" goes out of scope. Nit. That explanation of "ptr" might be better placed right at the add_random...() macro. > + */ > +void *__builtin_alloca(size_t size); > +/* > + * Use, at most, 10 bits of entropy. We explicitly cap this to keep the > + * "VLA" from being unbounded (see above). 10 bits leaves enough room for > + * per-arch offset masks to reduce entropy (by removing higher bits, since > + * high entropy may overly constrain usable stack space), and for > + * compiler/arch-specific stack alignment to remove the lower bits. > + */ > +#define KSTACK_OFFSET_MAX(x) ((x) & 0x3FF) > + > +/* > + * These macros must be used during syscall entry when interrupts and > + * preempt are disabled, and after user registers have been stored to > + * the stack. > + */ > +#define add_random_kstack_offset() do { \ > + if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \ > + &randomize_kstack_offset)) { \ > + u32 offset = __this_cpu_read(kstack_offset); \ > + u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset)); \ > + asm volatile("" : "=m"(*ptr) :: "memory"); \ > + } \ > +} while (0) Other than that. Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
On Wed, Mar 31, 2021 at 09:53:26AM +0200, Thomas Gleixner wrote: > On Tue, Mar 30 2021 at 13:57, Kees Cook wrote: > > +/* > > + * Do not use this anywhere else in the kernel. This is used here because > > + * it provides an arch-agnostic way to grow the stack with correct > > + * alignment. Also, since this use is being explicitly masked to a max of > > + * 10 bits, stack-clash style attacks are unlikely. For more details see > > + * "VLAs" in Documentation/process/deprecated.rst > > + * The asm statement is designed to convince the compiler to keep the > > + * allocation around even after "ptr" goes out of scope. > > Nit. That explanation of "ptr" might be better placed right at the > add_random...() macro. Ah, yes! Fixed in v9. > Other than that. > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> Thank you for the reviews! Do you want to take this via -tip (and leave off the arm64 patch until it is acked), or would you rather it go via arm64? (I've sent v9 now...)
On Wed, Mar 31 2021 at 14:54, Kees Cook wrote: > On Wed, Mar 31, 2021 at 09:53:26AM +0200, Thomas Gleixner wrote: >> On Tue, Mar 30 2021 at 13:57, Kees Cook wrote: >> > +/* >> > + * Do not use this anywhere else in the kernel. This is used here because >> > + * it provides an arch-agnostic way to grow the stack with correct >> > + * alignment. Also, since this use is being explicitly masked to a max of >> > + * 10 bits, stack-clash style attacks are unlikely. For more details see >> > + * "VLAs" in Documentation/process/deprecated.rst >> > + * The asm statement is designed to convince the compiler to keep the >> > + * allocation around even after "ptr" goes out of scope. >> >> Nit. That explanation of "ptr" might be better placed right at the >> add_random...() macro. > > Ah, yes! Fixed in v9. Hmm, looking at V9 the "ptr" thing got lost .... > +/* > + * Do not use this anywhere else in the kernel. This is used here because > + * it provides an arch-agnostic way to grow the stack with correct > + * alignment. Also, since this use is being explicitly masked to a max of > + * 10 bits, stack-clash style attacks are unlikely. For more details see > + * "VLAs" in Documentation/process/deprecated.rst > + */ > +void *__builtin_alloca(size_t size); > +/* > + * Use, at most, 10 bits of entropy. We explicitly cap this to keep the > + * "VLA" from being unbounded (see above). 10 bits leaves enough room for > + * per-arch offset masks to reduce entropy (by removing higher bits, since > + * high entropy may overly constrain usable stack space), and for > + * compiler/arch-specific stack alignment to remove the lower bits. > + */ > +#define KSTACK_OFFSET_MAX(x) ((x) & 0x3FF) > + > +/* > + * These macros must be used during syscall entry when interrupts and > + * preempt are disabled, and after user registers have been stored to > + * the stack. > + */ > +#define add_random_kstack_offset() do { \ > Do you want to take this via -tip (and leave off the arm64 patch until > it is acked), or would you rather it go via arm64? (I've sent v9 now...) Either way is fine. Thanks, tglx
On Thu, Apr 01, 2021 at 12:38:31AM +0200, Thomas Gleixner wrote: > On Wed, Mar 31 2021 at 14:54, Kees Cook wrote: > > On Wed, Mar 31, 2021 at 09:53:26AM +0200, Thomas Gleixner wrote: > >> On Tue, Mar 30 2021 at 13:57, Kees Cook wrote: > >> > +/* > >> > + * Do not use this anywhere else in the kernel. This is used here because > >> > + * it provides an arch-agnostic way to grow the stack with correct > >> > + * alignment. Also, since this use is being explicitly masked to a max of > >> > + * 10 bits, stack-clash style attacks are unlikely. For more details see > >> > + * "VLAs" in Documentation/process/deprecated.rst > >> > + * The asm statement is designed to convince the compiler to keep the > >> > + * allocation around even after "ptr" goes out of scope. > >> > >> Nit. That explanation of "ptr" might be better placed right at the > >> add_random...() macro. > > > > Ah, yes! Fixed in v9. > > Hmm, looking at V9 the "ptr" thing got lost .... I put the comment inline in the macro directly above the asm(). > > Do you want to take this via -tip (and leave off the arm64 patch until > > it is acked), or would you rather it go via arm64? (I've sent v9 now...) > > Either way is fine. Since the arm64 folks have been a bit busy, can you just put this in -tip and leave off the arm64 patch for now? Thanks!
On Tue, Mar 30, 2021 at 01:57:47PM -0700, Kees Cook wrote: > diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h > new file mode 100644 > index 000000000000..351520803006 > --- /dev/null > +++ b/include/linux/randomize_kstack.h > @@ -0,0 +1,55 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef _LINUX_RANDOMIZE_KSTACK_H > +#define _LINUX_RANDOMIZE_KSTACK_H > + > +#include <linux/kernel.h> > +#include <linux/jump_label.h> > +#include <linux/percpu-defs.h> > + > +DECLARE_STATIC_KEY_MAYBE(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, > + randomize_kstack_offset); > +DECLARE_PER_CPU(u32, kstack_offset); > + > +/* > + * Do not use this anywhere else in the kernel. This is used here because > + * it provides an arch-agnostic way to grow the stack with correct > + * alignment. Also, since this use is being explicitly masked to a max of > + * 10 bits, stack-clash style attacks are unlikely. For more details see > + * "VLAs" in Documentation/process/deprecated.rst > + * The asm statement is designed to convince the compiler to keep the > + * allocation around even after "ptr" goes out of scope. > + */ > +void *__builtin_alloca(size_t size); > +/* > + * Use, at most, 10 bits of entropy. We explicitly cap this to keep the > + * "VLA" from being unbounded (see above). 10 bits leaves enough room for > + * per-arch offset masks to reduce entropy (by removing higher bits, since > + * high entropy may overly constrain usable stack space), and for > + * compiler/arch-specific stack alignment to remove the lower bits. > + */ > +#define KSTACK_OFFSET_MAX(x) ((x) & 0x3FF) > + > +/* > + * These macros must be used during syscall entry when interrupts and > + * preempt are disabled, and after user registers have been stored to > + * the stack. > + */ > +#define add_random_kstack_offset() do { \ > + if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \ > + &randomize_kstack_offset)) { \ > + u32 offset = __this_cpu_read(kstack_offset); \ > + u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset)); \ > + asm volatile("" : "=m"(*ptr) :: "memory"); \ Using the "m" constraint here is dangerous if you don't actually evaluate it inside the asm. For example, if the compiler decides to generate an addressing mode relative to the stack but with writeback (autodecrement), then the stack pointer will be off by 8 bytes. Can you use "o" instead? Will
From: Will Deacon > Sent: 01 April 2021 09:31 ... > > +/* > > + * These macros must be used during syscall entry when interrupts and > > + * preempt are disabled, and after user registers have been stored to > > + * the stack. > > + */ > > +#define add_random_kstack_offset() do { \ > > + if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \ > > + &randomize_kstack_offset)) { \ > > + u32 offset = __this_cpu_read(kstack_offset); \ > > + u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset)); \ > > + asm volatile("" : "=m"(*ptr) :: "memory"); \ > > Using the "m" constraint here is dangerous if you don't actually evaluate it > inside the asm. For example, if the compiler decides to generate an > addressing mode relative to the stack but with writeback (autodecrement), then > the stack pointer will be off by 8 bytes. Can you use "o" instead? Is it allowed to use such a mode? It would have to know that the "m" was substituted exactly once. I think there are quite a few examples with 'strange' uses of memory asm arguments. However, in this case, isn't it enough to ensure the address is 'saved'? So: asm volatile("" : "=r"(ptr) ); should be enough. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Both Android and Chrome OS really want this feature; For Container-Optimized OS, we have customers interested in the defense too. Thank you very much. Change-Id: I1eb1b726007aa8f9c374b934cc1c690fb4924aa3
On Thu, Apr 01, 2021 at 12:17:44PM -0700, Roy Yang wrote: > Both Android and Chrome OS really want this feature; For Container-Optimized OS, we have customers > interested in the defense too. > > Thank you very much. > > Change-Id: I1eb1b726007aa8f9c374b934cc1c690fb4924aa3 You forgot to tell what patch you are refering to. Your Change-Id (whatever the hell that is) doesn't help at all. Don't assume that keys in your internal database make sense for the rest of the world, especially when they appear to contain a hash of something...
On Thu, Apr 01, 2021 at 07:48:30PM +0000, Al Viro wrote: > On Thu, Apr 01, 2021 at 12:17:44PM -0700, Roy Yang wrote: > > Both Android and Chrome OS really want this feature; For Container-Optimized OS, we have customers > > interested in the defense too. > > > > Thank you very much. > > > > Change-Id: I1eb1b726007aa8f9c374b934cc1c690fb4924aa3 > > You forgot to tell what patch you are refering to. Your > Change-Id (whatever the hell that is) doesn't help at all. Don't > assume that keys in your internal database make sense for the > rest of the world, especially when they appear to contain a hash > of something... The Change-Id fails to have any direct search hits at lore.kernel.org. However, it turn up Roy's original patch, and clicking on the message-Id in the "In-Reply-Field", it apperas Roy was replying to this message: https://lore.kernel.org/lkml/20210330205750.428816-1-keescook@chromium.org/ which is the head of this patch series: Subject: [PATCH v8 0/6] Optionally randomize kernel stack offset each syscall That being said, it would have been better if the original subject line had been preserved, and it's yet another example of how the lore.kernel.org URL is infinitely better than the Change-Id. :-) - Ted
On Thu, Apr 01, 2021 at 12:17:44PM -0700, Roy Yang wrote: > Both Android and Chrome OS really want this feature; For Container-Optimized OS, we have customers > interested in the defense too. It's pretty close! There are a couple recent comments that need to be addressed, but hopefully it can land if x86 and arm64 maintainers are happy v10. > Change-Id: I1eb1b726007aa8f9c374b934cc1c690fb4924aa3 > -- > 2.31.0.208.g409f899ff0-goog And to let other folks know, I'm guessing this email got sent with git send-email to try to get a valid In-Reply-To header, but I guess git trashed the Subject and ran hooks to generate a Change-Id UUID. I assume it's from following the "Reply instructions" at the bottom of: https://lore.kernel.org/lkml/20210330205750.428816-1-keescook@chromium.org/ (It seems those need clarification about Subject handling.)
On Thu, Apr 01, 2021 at 11:15:43AM +0000, David Laight wrote: > From: Will Deacon > > Sent: 01 April 2021 09:31 > ... > > > +/* > > > + * These macros must be used during syscall entry when interrupts and > > > + * preempt are disabled, and after user registers have been stored to > > > + * the stack. > > > + */ > > > +#define add_random_kstack_offset() do { \ > > > + if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \ > > > + &randomize_kstack_offset)) { \ > > > + u32 offset = __this_cpu_read(kstack_offset); \ > > > + u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset)); \ > > > + asm volatile("" : "=m"(*ptr) :: "memory"); \ > > > > Using the "m" constraint here is dangerous if you don't actually evaluate it > > inside the asm. For example, if the compiler decides to generate an > > addressing mode relative to the stack but with writeback (autodecrement), then > > the stack pointer will be off by 8 bytes. Can you use "o" instead? I see other examples of empty asm, but it's true, none are using "=m" read constraints. But, yes, using "o" appears to work happily. > Is it allowed to use such a mode? > It would have to know that the "m" was substituted exactly once. > I think there are quite a few examples with 'strange' uses of memory > asm arguments. > > However, in this case, isn't it enough to ensure the address is 'saved'? > So: > asm volatile("" : "=r"(ptr) ); > should be enough. It isn't, it seems. Here's a comparison: https://godbolt.org/z/xYGn9GfGY So, I'll resend with "o", and with raw_cpu_*(). Thanks!