All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] stack: replace "o" output with "r" input constraint
@ 2021-04-19 23:17 Kees Cook
  2021-04-23  1:02 ` Nathan Chancellor
  2021-05-11  8:02 ` [tip: core/urgent] stack: Replace " tip-bot2 for Nick Desaulniers
  0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2021-04-19 23:17 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Kees Cook, Nick Desaulniers, Nathan Chancellor, Elena Reshetova,
	David Laight, Will Deacon, clang-built-linux, linux-kernel

From: Nick Desaulniers <ndesaulniers@google.com>

"o" isn't a common asm() constraint to use; it triggers an assertion in
assert-enabled builds of LLVM that it's not recognized when targeting
aarch64 (though it appears to fall back to "m"). I've fixed this in LLVM
13 now, but there isn't really a good reason to be using "o" in particular
here. To avoid causing build issues for those using assert-enabled builds
of earlier LLVM versions, the constraint needs changing.

Instead, if the point is to retain the __builtin_alloca(), we can make ptr
appear to "escape" via being an input to an empty inline asm block. This
is preferable anyways, since otherwise this looks like a dead store.

While the use of "r" was considered in
https://lore.kernel.org/lkml/202104011447.2E7F543@keescook/
it was only tested as an output (which looks like a dead store, and
wasn't sufficient). Use "r" as an input constraint instead, which
behaves correctly across compilers and architectures:
https://godbolt.org/z/E9cd411ob

Link: https://reviews.llvm.org/D100412
Link: https://bugs.llvm.org/show_bug.cgi?id=49956
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Kees Cook <keescook@chromium.org>
Fixes: 39218ff4c625 ("stack: Optionally randomize kernel stack offset each syscall")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/randomize_kstack.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
index fd80fab663a9..bebc911161b6 100644
--- a/include/linux/randomize_kstack.h
+++ b/include/linux/randomize_kstack.h
@@ -38,7 +38,7 @@ void *__builtin_alloca(size_t size);
 		u32 offset = raw_cpu_read(kstack_offset);		\
 		u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset));	\
 		/* Keep allocation even after "ptr" loses scope. */	\
-		asm volatile("" : "=o"(*ptr) :: "memory");		\
+		asm volatile("" :: "r"(ptr) : "memory");		\
 	}								\
 } while (0)
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] stack: replace "o" output with "r" input constraint
  2021-04-19 23:17 [PATCH] stack: replace "o" output with "r" input constraint Kees Cook
@ 2021-04-23  1:02 ` Nathan Chancellor
  2021-05-05 21:12   ` Nathan Chancellor
  2021-05-11  8:02 ` [tip: core/urgent] stack: Replace " tip-bot2 for Nick Desaulniers
  1 sibling, 1 reply; 5+ messages in thread
From: Nathan Chancellor @ 2021-04-23  1:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Thomas Gleixner, Nick Desaulniers, Elena Reshetova, David Laight,
	Will Deacon, clang-built-linux, linux-kernel

On Mon, Apr 19, 2021 at 04:17:41PM -0700, Kees Cook wrote:
> From: Nick Desaulniers <ndesaulniers@google.com>
> 
> "o" isn't a common asm() constraint to use; it triggers an assertion in
> assert-enabled builds of LLVM that it's not recognized when targeting
> aarch64 (though it appears to fall back to "m"). I've fixed this in LLVM
> 13 now, but there isn't really a good reason to be using "o" in particular
> here. To avoid causing build issues for those using assert-enabled builds
> of earlier LLVM versions, the constraint needs changing.
> 
> Instead, if the point is to retain the __builtin_alloca(), we can make ptr
> appear to "escape" via being an input to an empty inline asm block. This
> is preferable anyways, since otherwise this looks like a dead store.
> 
> While the use of "r" was considered in
> https://lore.kernel.org/lkml/202104011447.2E7F543@keescook/
> it was only tested as an output (which looks like a dead store, and
> wasn't sufficient). Use "r" as an input constraint instead, which
> behaves correctly across compilers and architectures:
> https://godbolt.org/z/E9cd411ob
> 
> Link: https://reviews.llvm.org/D100412
> Link: https://bugs.llvm.org/show_bug.cgi?id=49956
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Fixes: 39218ff4c625 ("stack: Optionally randomize kernel stack offset each syscall")
> Signed-off-by: Kees Cook <keescook@chromium.org>

I built arm64 defconfig with and without
CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT with LLVM 12 (which does not have
Nick's LLVM fix) without any issues and did a quick boot test in QEMU,
nothing exploded.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  include/linux/randomize_kstack.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
> index fd80fab663a9..bebc911161b6 100644
> --- a/include/linux/randomize_kstack.h
> +++ b/include/linux/randomize_kstack.h
> @@ -38,7 +38,7 @@ void *__builtin_alloca(size_t size);
>  		u32 offset = raw_cpu_read(kstack_offset);		\
>  		u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset));	\
>  		/* Keep allocation even after "ptr" loses scope. */	\
> -		asm volatile("" : "=o"(*ptr) :: "memory");		\
> +		asm volatile("" :: "r"(ptr) : "memory");		\
>  	}								\
>  } while (0)
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] stack: replace "o" output with "r" input constraint
  2021-04-23  1:02 ` Nathan Chancellor
@ 2021-05-05 21:12   ` Nathan Chancellor
  2021-05-05 21:19     ` Kees Cook
  0 siblings, 1 reply; 5+ messages in thread
From: Nathan Chancellor @ 2021-05-05 21:12 UTC (permalink / raw)
  To: Kees Cook
  Cc: Thomas Gleixner, Nick Desaulniers, Elena Reshetova, David Laight,
	Will Deacon, clang-built-linux, linux-kernel

On Thu, Apr 22, 2021 at 06:02:27PM -0700, Nathan Chancellor wrote:
> On Mon, Apr 19, 2021 at 04:17:41PM -0700, Kees Cook wrote:
> > From: Nick Desaulniers <ndesaulniers@google.com>
> > 
> > "o" isn't a common asm() constraint to use; it triggers an assertion in
> > assert-enabled builds of LLVM that it's not recognized when targeting
> > aarch64 (though it appears to fall back to "m"). I've fixed this in LLVM
> > 13 now, but there isn't really a good reason to be using "o" in particular
> > here. To avoid causing build issues for those using assert-enabled builds
> > of earlier LLVM versions, the constraint needs changing.
> > 
> > Instead, if the point is to retain the __builtin_alloca(), we can make ptr
> > appear to "escape" via being an input to an empty inline asm block. This
> > is preferable anyways, since otherwise this looks like a dead store.
> > 
> > While the use of "r" was considered in
> > https://lore.kernel.org/lkml/202104011447.2E7F543@keescook/
> > it was only tested as an output (which looks like a dead store, and
> > wasn't sufficient). Use "r" as an input constraint instead, which
> > behaves correctly across compilers and architectures:
> > https://godbolt.org/z/E9cd411ob
> > 
> > Link: https://reviews.llvm.org/D100412
> > Link: https://bugs.llvm.org/show_bug.cgi?id=49956
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > Tested-by: Kees Cook <keescook@chromium.org>
> > Fixes: 39218ff4c625 ("stack: Optionally randomize kernel stack offset each syscall")

Kees, were you planning on taking this to Linus or someone else? It
would be nice to have this in for -rc1 (although I understand it might
be too late), if not, by -rc2.

Cheers,
Nathan

> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> I built arm64 defconfig with and without
> CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT with LLVM 12 (which does not have
> Nick's LLVM fix) without any issues and did a quick boot test in QEMU,
> nothing exploded.
> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> 
> > ---
> >  include/linux/randomize_kstack.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
> > index fd80fab663a9..bebc911161b6 100644
> > --- a/include/linux/randomize_kstack.h
> > +++ b/include/linux/randomize_kstack.h
> > @@ -38,7 +38,7 @@ void *__builtin_alloca(size_t size);
> >  		u32 offset = raw_cpu_read(kstack_offset);		\
> >  		u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset));	\
> >  		/* Keep allocation even after "ptr" loses scope. */	\
> > -		asm volatile("" : "=o"(*ptr) :: "memory");		\
> > +		asm volatile("" :: "r"(ptr) : "memory");		\
> >  	}								\
> >  } while (0)
> >  
> > -- 
> > 2.25.1
> > 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] stack: replace "o" output with "r" input constraint
  2021-05-05 21:12   ` Nathan Chancellor
@ 2021-05-05 21:19     ` Kees Cook
  0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2021-05-05 21:19 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Thomas Gleixner, Nick Desaulniers, Elena Reshetova, David Laight,
	Will Deacon, clang-built-linux, linux-kernel

On Wed, May 05, 2021 at 02:12:32PM -0700, Nathan Chancellor wrote:
> On Thu, Apr 22, 2021 at 06:02:27PM -0700, Nathan Chancellor wrote:
> > On Mon, Apr 19, 2021 at 04:17:41PM -0700, Kees Cook wrote:
> > > From: Nick Desaulniers <ndesaulniers@google.com>
> > > 
> > > "o" isn't a common asm() constraint to use; it triggers an assertion in
> > > assert-enabled builds of LLVM that it's not recognized when targeting
> > > aarch64 (though it appears to fall back to "m"). I've fixed this in LLVM
> > > 13 now, but there isn't really a good reason to be using "o" in particular
> > > here. To avoid causing build issues for those using assert-enabled builds
> > > of earlier LLVM versions, the constraint needs changing.
> > > 
> > > Instead, if the point is to retain the __builtin_alloca(), we can make ptr
> > > appear to "escape" via being an input to an empty inline asm block. This
> > > is preferable anyways, since otherwise this looks like a dead store.
> > > 
> > > While the use of "r" was considered in
> > > https://lore.kernel.org/lkml/202104011447.2E7F543@keescook/
> > > it was only tested as an output (which looks like a dead store, and
> > > wasn't sufficient). Use "r" as an input constraint instead, which
> > > behaves correctly across compilers and architectures:
> > > https://godbolt.org/z/E9cd411ob
> > > 
> > > Link: https://reviews.llvm.org/D100412
> > > Link: https://bugs.llvm.org/show_bug.cgi?id=49956
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Tested-by: Kees Cook <keescook@chromium.org>
> > > Fixes: 39218ff4c625 ("stack: Optionally randomize kernel stack offset each syscall")
> 
> Kees, were you planning on taking this to Linus or someone else? It
> would be nice to have this in for -rc1 (although I understand it might
> be too late), if not, by -rc2.

I assumed Thomas would pick this up. Thomas, shall I send this directly
to Linus?

Thanks!

-Kees

> 
> Cheers,
> Nathan
> 
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > 
> > I built arm64 defconfig with and without
> > CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT with LLVM 12 (which does not have
> > Nick's LLVM fix) without any issues and did a quick boot test in QEMU,
> > nothing exploded.
> > 
> > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> > 
> > > ---
> > >  include/linux/randomize_kstack.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
> > > index fd80fab663a9..bebc911161b6 100644
> > > --- a/include/linux/randomize_kstack.h
> > > +++ b/include/linux/randomize_kstack.h
> > > @@ -38,7 +38,7 @@ void *__builtin_alloca(size_t size);
> > >  		u32 offset = raw_cpu_read(kstack_offset);		\
> > >  		u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset));	\
> > >  		/* Keep allocation even after "ptr" loses scope. */	\
> > > -		asm volatile("" : "=o"(*ptr) :: "memory");		\
> > > +		asm volatile("" :: "r"(ptr) : "memory");		\
> > >  	}								\
> > >  } while (0)
> > >  
> > > -- 
> > > 2.25.1
> > > 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip: core/urgent] stack: Replace "o" output with "r" input constraint
  2021-04-19 23:17 [PATCH] stack: replace "o" output with "r" input constraint Kees Cook
  2021-04-23  1:02 ` Nathan Chancellor
@ 2021-05-11  8:02 ` tip-bot2 for Nick Desaulniers
  1 sibling, 0 replies; 5+ messages in thread
From: tip-bot2 for Nick Desaulniers @ 2021-05-11  8:02 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Nick Desaulniers, Kees Cook, Thomas Gleixner, Nathan Chancellor,
	x86, linux-kernel

The following commit has been merged into the core/urgent branch of tip:

Commit-ID:     2515dd6ce8e545b0b2eece84920048ef9ed846c4
Gitweb:        https://git.kernel.org/tip/2515dd6ce8e545b0b2eece84920048ef9ed846c4
Author:        Nick Desaulniers <ndesaulniers@google.com>
AuthorDate:    Mon, 19 Apr 2021 16:17:41 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 11 May 2021 09:56:11 +02:00

stack: Replace "o" output with "r" input constraint

"o" isn't a common asm() constraint to use; it triggers an assertion in
assert-enabled builds of LLVM that it's not recognized when targeting
aarch64 (though it appears to fall back to "m"). It's fixed in LLVM 13 now,
but there isn't really a good reason to use "o" in particular here. To
avoid causing build issues for those using assert-enabled builds of earlier
LLVM versions, the constraint needs changing.

Instead, if the point is to retain the __builtin_alloca(), make ptr appear
to "escape" via being an input to an empty inline asm block. This is
preferable anyways, since otherwise this looks like a dead store.

While the use of "r" was considered in

  https://lore.kernel.org/lkml/202104011447.2E7F543@keescook/

it was only tested as an output (which looks like a dead store, and wasn't
sufficient).

Use "r" as an input constraint instead, which behaves correctly across
compilers and architectures.

Fixes: 39218ff4c625 ("stack: Optionally randomize kernel stack offset each syscall")
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kees Cook <keescook@chromium.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Link: https://reviews.llvm.org/D100412
Link: https://bugs.llvm.org/show_bug.cgi?id=49956
Link: https://lore.kernel.org/r/20210419231741.4084415-1-keescook@chromium.org

---
 include/linux/randomize_kstack.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
index fd80fab..bebc911 100644
--- a/include/linux/randomize_kstack.h
+++ b/include/linux/randomize_kstack.h
@@ -38,7 +38,7 @@ void *__builtin_alloca(size_t size);
 		u32 offset = raw_cpu_read(kstack_offset);		\
 		u8 *ptr = __builtin_alloca(KSTACK_OFFSET_MAX(offset));	\
 		/* Keep allocation even after "ptr" loses scope. */	\
-		asm volatile("" : "=o"(*ptr) :: "memory");		\
+		asm volatile("" :: "r"(ptr) : "memory");		\
 	}								\
 } while (0)
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-05-11  8:02 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 23:17 [PATCH] stack: replace "o" output with "r" input constraint Kees Cook
2021-04-23  1:02 ` Nathan Chancellor
2021-05-05 21:12   ` Nathan Chancellor
2021-05-05 21:19     ` Kees Cook
2021-05-11  8:02 ` [tip: core/urgent] stack: Replace " tip-bot2 for Nick Desaulniers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.