linux-kernel - Re: [PATCH] remove AND operation in choose_random_kstack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 16:52:15 +0100
From: Mark Rutland <mark.rutland@....com>
To: Yuntao Liu <liuyuntao12@...wei.com>
Cc: x86@...nel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
	linux-hardening@...r.kernel.org, catalin.marinas@....com,
	will@...nel.org, hca@...ux.ibm.com, gor@...ux.ibm.com,
	agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
	svens@...ux.ibm.com, tglx@...utronix.de, mingo@...hat.com,
	bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
	kees@...nel.org, gustavoars@...nel.org, arnd@...db.de,
	leobras@...hat.com, broonie@...nel.org, imbrenda@...ux.ibm.com,
	pawan.kumar.gupta@...ux.intel.com
Subject: Re: [PATCH] remove AND operation in choose_random_kstack_offset()

On Mon, Jun 17, 2024 at 01:37:21PM +0000, Yuntao Liu wrote:
> Since the offset would be bitwise ANDed with 0x3FF in
> add_random_kstack_offset(), so just remove AND operation here.
> 
> Signed-off-by: Yuntao Liu <liuyuntao12@...wei.com>

The comments in arm64 and x86 say that they're deliberately capping the
offset at fewer bits than the result of KSTACK_OFFSET_MAX() masking the
value with 0x3FF.

Maybe it's ok to expand that, but if that's the case the commit message
needs to explain why it's safe add extra bits (2 on arm64, 3 on s39 and
x86), and those comments need to be updated accordingly.

As-is, I do not think this patch is ok.

Mark.

> ---
>  arch/arm64/kernel/syscall.c          | 2 +-
>  arch/s390/include/asm/entry-common.h | 2 +-
>  arch/x86/include/asm/entry-common.h  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> index ad198262b981..43f555f7cd2d 100644
> --- a/arch/arm64/kernel/syscall.c
> +++ b/arch/arm64/kernel/syscall.c
> @@ -63,7 +63,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
>  	 *
>  	 * The resulting 5 bits of entropy is seen in SP[8:4].
>  	 */
> -	choose_random_kstack_offset(get_random_u16() & 0x1FF);
> +	choose_random_kstack_offset(get_random_u16());
>  }
>  
>  static inline bool has_syscall_work(unsigned long flags)
> diff --git a/arch/s390/include/asm/entry-common.h b/arch/s390/include/asm/entry-common.h
> index 7f5004065e8a..35555c944630 100644
> --- a/arch/s390/include/asm/entry-common.h
> +++ b/arch/s390/include/asm/entry-common.h
> @@ -54,7 +54,7 @@ static __always_inline void arch_exit_to_user_mode(void)
>  static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>  						  unsigned long ti_work)
>  {
> -	choose_random_kstack_offset(get_tod_clock_fast() & 0xff);
> +	choose_random_kstack_offset(get_tod_clock_fast());
>  }
>  
>  #define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare
> diff --git a/arch/x86/include/asm/entry-common.h b/arch/x86/include/asm/entry-common.h
> index 7e523bb3d2d3..b28a307f2014 100644
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -85,7 +85,7 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>  	 * Therefore, final stack offset entropy will be 5 (x86_64) or
>  	 * 6 (ia32) bits.
>  	 */
> -	choose_random_kstack_offset(rdtsc() & 0xFF);
> +	choose_random_kstack_offset(rdtsc());
>  }
>  #define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare
>  
> -- 
> 2.34.1
>