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]
Message-ID: <ZnBbr2CAqBGDe2aN@J2N7QTR9R3>
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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ