linux-kernel - Re: [PATCH 2/4] x86/signal: Use switch_to_permissive_pkey

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] [day] [month] [year] [list]

Message-ID: <d6d7745f-ad25-4faa-a3a3-65a4604e8592@intel.com>
Date: Fri, 21 Feb 2025 08:26:46 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Dmitry Vyukov <dvyukov@...gle.com>, mathieu.desnoyers@...icios.com,
 peterz@...radead.org, boqun.feng@...il.com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
 aruna.ramakrishna@...cle.com, elver@...gle.com
Cc: "Paul E. McKenney" <paulmck@...nel.org>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] x86/signal: Use switch_to_permissive_pkey_reg()
 helper

...
> -/*
> - * Enable all pkeys temporarily, so as to ensure that both the current
> - * execution stack as well as the alternate signal stack are writeable.
> - * The application can use any of the available pkeys to protect the
> - * alternate signal stack, and we don't know which one it is, so enable
> - * all. The PKRU register will be reset to init_pkru later in the flow,
> - * in fpu__clear_user_states(), and it is the application's responsibility
> - * to enable the appropriate pkey as the first step in the signal handler
> - * so that the handler does not segfault.
> - */
> -static inline u32 sig_prepare_pkru(void)
> -{
> -	u32 orig_pkru = read_pkru();
> -
> -	write_pkru(0);
> -	return orig_pkru;
> -}
> -
>  /*
>   * Set up a signal frame.
>   */
> @@ -157,8 +140,18 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
>  		return (void __user *)-1L;
>  	}
>  
> -	/* Update PKRU to enable access to the alternate signal stack. */
> -	pkru = sig_prepare_pkru();
> +	/*
> +	 * Enable all pkeys temporarily, so as to ensure that both the current
> +	 * execution stack as well as the alternate signal stack are
> +	 * writeable. The application can use any of the available pkeys to
> +	 * protect the alternate signal stack, and we don't know which one it
> +	 * is, so enable all. The PKRU register will be reset to init_pkru
> +	 * later in the flow, in fpu__clear_user_states(), and it is the
> +	 * application's responsibility to enable the appropriate pkey as the
> +	 * first step in the signal handler so that the handler does not
> +	 * segfault.
> +	 */
> +	pkru = switch_to_permissive_pkey_reg();
I think this hurts readability too much in the get_sigframe() code. On
some level, it's silly to have a basically empty helper. But in this
case, it does help keep the signal code readable.

In other words, this would be preferred:

/*
 * Keep existing big comment
 */
static inline u32 sig_prepare_pkru(void)
{
	return switch_to_permissive_pkey_reg();
}