linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Borislav Petkov <bp@suse.de>
To: "Chang S. Bae" <chang.seok.bae@intel.com>
Cc: tglx@linutronix.de, mingo@kernel.org, luto@kernel.org,
	x86@kernel.org, len.brown@intel.com, dave.hansen@intel.com,
	hjl.tools@gmail.com, Dave.Martin@arm.com, jannh@google.com,
	mpe@ellerman.id.au, carlos@redhat.com, tony.luck@intel.com,
	ravi.v.shankar@intel.com, libc-alpha@sourceware.org,
	linux-arch@vger.kernel.org, linux-api@vger.kernel.org,
	linux-kernel@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ
Date: Fri, 5 Mar 2021 11:43:25 +0100	[thread overview]
Message-ID: <20210305104325.GA2896@zn.tnic> (raw)
In-Reply-To: <20210227165911.32757-4-chang.seok.bae@intel.com>

On Sat, Feb 27, 2021 at 08:59:08AM -0800, Chang S. Bae wrote:
> Historically, signal.h defines MINSIGSTKSZ (2KB) and SIGSTKSZ (8KB), for
> use by all architectures with sigaltstack(2). Over time, the hardware state
> size grew, but these constants did not evolve. Today, literal use of these
> constants on several architectures may result in signal stack overflow, and
> thus user data corruption.
> 
> A few years ago, the ARM team addressed this issue by establishing
> getauxval(AT_MINSIGSTKSZ). This enables the kernel to supply at runtime
> value that is an appropriate replacement on the current and future
> hardware.
> 
> Add getauxval(AT_MINSIGSTKSZ) support to x86, analogous to the support
> added for ARM in commit 94b07c1f8c39 ("arm64: signal: Report signal frame
> size to userspace via auxv").
> 
> Also, include a documentation to describe x86-specific auxiliary vectors.
> 
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")

Right, so this has a Fixes: tag and points to bugzilla entry which talks
about signal stack corruption with AVX-512F.

But if this is going to be backported to stable, then the patch(es)
should be minimal and not contain documentation. And if so, one will
need all three to be backported, which means, a cc:stable should contain
a comment explaining that.

Or am I misreading and they should not need to be backported to stable
because some <non-obvious reason>?

Also, I'm not sure backporting a patch to stable which changes ABI is
ok. It probably is but I don't know.

So what's the deal here?

> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Cc: H.J. Lu <hjl.tools@gmail.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: x86@kernel.org
> Cc: libc-alpha@sourceware.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-api@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
> ---
> Changes from v5:
> * Added a documentation.
> ---
>  Documentation/x86/elf_auxvec.rst   | 56 ++++++++++++++++++++++++++++++
>  arch/x86/include/asm/elf.h         |  4 +++
>  arch/x86/include/uapi/asm/auxvec.h |  4 +--
>  arch/x86/kernel/signal.c           |  5 +++
>  4 files changed, 67 insertions(+), 2 deletions(-)

You also need:

diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
index 4693e192b447..d58614d5cde6 100644
--- a/Documentation/x86/index.rst
+++ b/Documentation/x86/index.rst
@@ -35,3 +35,4 @@ x86-specific Documentation
    sva
    sgx
    features
+   elf_auxvec

to add this to the TOC.

>  create mode 100644 Documentation/x86/elf_auxvec.rst
> 
> diff --git a/Documentation/x86/elf_auxvec.rst b/Documentation/x86/elf_auxvec.rst
> new file mode 100644
> index 000000000000..751c552c4048
> --- /dev/null
> +++ b/Documentation/x86/elf_auxvec.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==================================
> +x86-specific ELF Auxiliary Vectors
> +==================================
> +
> +This document describes the semantics of the x86 auxiliary vectors.
> +
> +1. Introduction
> +---------------
> +
> +ELF Auxiliary vectors enable the kernel to efficiently provide
> +configuration specific parameters to userspace. In this example, a program
> +allocates an alternate stack based on the kernel-provided size.
> +
> +   #include <sys/auxv.h>
> +   #include <elf.h>
> +
> +   #ifndef AT_MINSIGSTKSZ
> +   #define AT_MINSIGSTKSZ	51
> +   #endif
> +
> +   stack_t ss;
> +   int err;
> +
> +   ss.ss_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
> +   ss.ss_sp = malloc(ss.ss_size);
> +   ...
> +
> +   err = sigaltstack(&ss, NULL);
> +   ...

That source code needs some special markup to look like source code -
currently, the result looks bad.

> +
> +
> +2. The exposed auxiliary vectors
> +---------------------------------
> +
> +AT_SYSINFO
> +    The entry point to the system call function the virtual Dynamic Shared
> +    Object (vDSO), not exported on 64-bit.

I can't parse that sentence.

> +
> +AT_SYSINFO_EHDR
> +    The start address of the page containing vDSO.
						^
						the


> +
> +AT_MINSIGSTKSZ
> +    The minimum stack size required to deliver a signal. It is a calculated
> +    sigframe size based on the largest possible user context. When programs
> +    use sigaltstack() to provide alternate signal stack, that stack must be
> +    at least the size to function properly on this hardware. Note that this
> +    is a minimum of the kernel to correctly get to the signal handler.

I get what this is trying to say but it reads weird. Simplify pls.

> +    Additional space must be added to handle objects pushed onto the stack
> +    by the signal handlers, as well as for nested signal delivery.
> +
> +    The purpose of this parameter is to accommodate the different stack
> +    sizes required by different hardware configuration. E.g., the x86
> +    system supporting the Advanced Vector Extension needs at least 8KB more
> +    than the one without it.

That could be simplified too.

> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 66bdfe838d61..cd10795c178e 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -312,6 +312,7 @@ do {									\
>  		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
>  		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
>  	}								\
> +	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\

Check vertical alignment of the '\'

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

  reply	other threads:[~2021-03-05 10:44 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-27 16:59 [PATCH v6 0/6] x86: Improve Minimum Alternate Stack Size Chang S. Bae
2021-02-27 16:59 ` [PATCH v6 1/6] uapi: Define the aux vector AT_MINSIGSTKSZ Chang S. Bae
2021-03-01 19:09   ` Borislav Petkov
2021-03-10 16:31     ` Bae, Chang Seok
2021-02-27 16:59 ` [PATCH v6 2/6] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
2021-02-27 16:59 ` [PATCH v6 3/6] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ Chang S. Bae
2021-03-05 10:43   ` Borislav Petkov [this message]
2021-03-10 16:34     ` Bae, Chang Seok
2021-03-10 17:43       ` Borislav Petkov
2021-03-10 18:01         ` Bae, Chang Seok
2021-02-27 16:59 ` [PATCH v6 4/6] selftest/sigaltstack: Use the AT_MINSIGSTKSZ aux vector if available Chang S. Bae
2021-02-27 16:59 ` [PATCH v6 5/6] x86/signal: Detect and prevent an alternate signal stack overflow Chang S. Bae
2021-02-27 16:59 ` [PATCH v6 6/6] selftest/x86/signal: Include test cases for validating sigaltstack Chang S. Bae

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=20210305104325.GA2896@zn.tnic \
    --to=bp@suse.de \
    --cc=Dave.Martin@arm.com \
    --cc=carlos@redhat.com \
    --cc=chang.seok.bae@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=fenghua.yu@intel.com \
    --cc=hjl.tools@gmail.com \
    --cc=jannh@google.com \
    --cc=len.brown@intel.com \
    --cc=libc-alpha@sourceware.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=ravi.v.shankar@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).