linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arvind Sankar <nivedita@alum.mit.edu>
To: Kees Cook <keescook@chromium.org>
Cc: Arvind Sankar <nivedita@alum.mit.edu>,
	Thomas Gleixner <tglx@linutronix.de>,
	Elena Reshetova <elena.reshetova@intel.com>,
	x86@kernel.org, Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Potapenko <glider@google.com>,
	Alexander Popov <alex.popov@linux.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Jann Horn <jannh@google.com>,
	kernel-hardening@lists.openwall.com,
	linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/5] stack: Optionally randomize kernel stack offset each syscall
Date: Mon, 22 Jun 2020 20:05:10 -0400	[thread overview]
Message-ID: <20200623000510.GA3542245@rani.riverdale.lan> (raw)
In-Reply-To: <202006221604.871B13DE3@keescook>

On Mon, Jun 22, 2020 at 04:07:11PM -0700, Kees Cook wrote:
> On Mon, Jun 22, 2020 at 06:56:15PM -0400, Arvind Sankar wrote:
> > On Mon, Jun 22, 2020 at 12:31:44PM -0700, Kees Cook wrote:
> > > +
> > > +#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(offset & 0x3FF);		\
> > > +		asm volatile("" : "=m"(*ptr));				\
> > > +	}								\
> > > +} while (0)
> > 
> > This feels a little fragile. ptr doesn't escape the block, so the
> > compiler is free to restore the stack immediately after this block. In
> > fact, given that all you've said is that the asm modifies *ptr, but
> > nothing uses that output, the compiler could eliminate the whole thing,
> > no?
> > 
> > https://godbolt.org/z/HT43F5
> > 
> > gcc restores the stack immediately, if no function calls come after it.
> > 
> > clang completely eliminates the code if no function calls come after.
> 
> nothing uses the stack in your example. And adding a barrier (which is
> what the "=m" is, doesn't change it.

Yeah, I realized that that was what's going on. And clang isn't actually
DCE'ing it, it's taking advantage of the red zone since my alloca was
small enough.

But I still don't see anything _stopping_ the compiler from optimizing
this better in the future. The "=m" is not a barrier: it just informs
the compiler that the asm produces an output value in *ptr (and no other
outputs). If nothing can consume that output, it doesn't stop the
compiler from freeing the allocation immediately after the asm instead
of at the end of the function.

I'm talking about something like
	asm volatile("" : : "r" (ptr) : "memory");
which tells the compiler that the asm may change memory arbitrarily.

Here, we don't use it really as a barrier, but to tell the compiler that
the asm may have stashed the value of ptr somewhere in memory, so it's
not free to reuse the space that it pointed to until the function
returns (unless it can prove that nothing accesses memory, not just that
nothing accesses ptr).

  reply	other threads:[~2020-06-23  0:05 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-22 19:31 [PATCH v4 0/5] " Kees Cook
2020-06-22 19:31 ` [PATCH v4 1/5] jump_label: Provide CONFIG-driven build state defaults Kees Cook
2020-06-22 19:31 ` [PATCH v4 2/5] init_on_alloc: Unpessimize default-on builds Kees Cook
2020-06-22 19:31 ` [PATCH v4 3/5] stack: Optionally randomize kernel stack offset each syscall Kees Cook
2020-06-22 19:40   ` Randy Dunlap
2020-06-22 21:26     ` Kees Cook
2020-06-22 20:07   ` Jann Horn
2020-06-22 21:30     ` Kees Cook
2020-06-22 21:42       ` Jann Horn
2020-06-22 22:04         ` Kees Cook
2020-06-22 22:56   ` Arvind Sankar
2020-06-22 23:07     ` Kees Cook
2020-06-23  0:05       ` Arvind Sankar [this message]
2020-06-23  0:56         ` Kees Cook
2020-06-23 13:42           ` David Laight
2020-06-23 12:38   ` Alexander Popov
2020-06-22 19:31 ` [PATCH v4 4/5] x86/entry: Enable random_kstack_offset support Kees Cook
2020-06-22 19:31 ` [PATCH v4 5/5] arm64: entry: " Kees Cook
2020-06-23  9:40   ` Mark Rutland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200623000510.GA3542245@rani.riverdale.lan \
    --to=nivedita@alum.mit.edu \
    --cc=alex.popov@linux.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=elena.reshetova@intel.com \
    --cc=glider@google.com \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v4 3/5] stack: Optionally randomize kernel stack offset each syscall' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).