Re: [PATCH v4 2/4] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ

[Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org>]

On 1/15/21 4:10 PM, Chang S. Bae via Libc-alpha 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), such that the kernel can 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").

The value of AT_MINSIGSTKSZ has been generic ABI for all architectures since
2018 when it was added to glibc's generic elf.h because of arm64.

Rather than define this just for x86 may we please put this into the generic
headers, since it has been there since 2018?

Any userspace code that might be inspecting for AT_MINSIGSTKSZ is going to
expect a value of 51.

No other architecture has a define for value 51. This way we avoid any accidents
with the value being different for architectures.
 
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Fixes: c2bc11f10a39 ("x86, AVX-512: Enable AVX-512 States Context Switch")
> 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-kernel@vger.kernel.org
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=153531
> ---
>  arch/x86/include/asm/elf.h         | 4 ++++
>  arch/x86/include/uapi/asm/auxvec.h | 6 ++++--
>  arch/x86/kernel/signal.c           | 5 +++++
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index b9a5d488f1a5..044b024abea1 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -311,6 +311,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());			\
>  } while (0)
>  
>  /*
> @@ -327,6 +328,7 @@ extern unsigned long task_size_32bit(void);
>  extern unsigned long task_size_64bit(int full_addr_space);
>  extern unsigned long get_mmap_base(int is_legacy);
>  extern bool mmap_address_hint_valid(unsigned long addr, unsigned long len);
> +extern unsigned long get_sigframe_size(void);
>  
>  #ifdef CONFIG_X86_32
>  
> @@ -348,6 +350,7 @@ do {									\
>  	if (vdso64_enabled)						\
>  		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
>  			    (unsigned long __force)current->mm->context.vdso); \
> +	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
>  } while (0)
>  
>  /* As a historical oddity, the x32 and x86_64 vDSOs are controlled together. */
> @@ -356,6 +359,7 @@ do {									\
>  	if (vdso64_enabled)						\
>  		NEW_AUX_ENT(AT_SYSINFO_EHDR,				\
>  			    (unsigned long __force)current->mm->context.vdso); \
> +	NEW_AUX_ENT(AT_MINSIGSTKSZ, get_sigframe_size());			\
>  } while (0)
>  
>  #define AT_SYSINFO		32
> diff --git a/arch/x86/include/uapi/asm/auxvec.h b/arch/x86/include/uapi/asm/auxvec.h
> index 580e3c567046..edd7808060e6 100644
> --- a/arch/x86/include/uapi/asm/auxvec.h
> +++ b/arch/x86/include/uapi/asm/auxvec.h
> @@ -10,11 +10,13 @@
>  #endif
>  #define AT_SYSINFO_EHDR		33
>  
> +#define AT_MINSIGSTKSZ		51

This definition should go into the generic auxvec.h.

We could also remove the arm64 define, but that is orthogonal arch cleanup.

> +
>  /* entries in ARCH_DLINFO: */
>  #if defined(CONFIG_IA32_EMULATION) || !defined(CONFIG_X86_64)
> -# define AT_VECTOR_SIZE_ARCH 2
> +# define AT_VECTOR_SIZE_ARCH 3
>  #else /* else it's non-compat x86-64 */
> -# define AT_VECTOR_SIZE_ARCH 1
> +# define AT_VECTOR_SIZE_ARCH 2
>  #endif
>  
>  #endif /* _ASM_X86_AUXVEC_H */
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 138a9f5b78d8..761d856f8ef7 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -716,6 +716,11 @@ void __init init_sigframe_size(void)
>  	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
>  }
>  
> +unsigned long get_sigframe_size(void)
> +{
> +	return max_frame_size;
> +}
> +
>  static inline int is_ia32_compat_frame(struct ksignal *ksig)
>  {
>  	return IS_ENABLED(CONFIG_IA32_EMULATION) &&
> 


-- 
Cheers,
Carlos.