linux-kernel - Re: [PATCH 3/4] rseq: Make rseq work with protection keys

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: <81d94ec3-16af-45a7-87c6-ef76570953f8@intel.com>
Date: Fri, 21 Feb 2025 09:17:26 -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 3/4] rseq: Make rseq work with protection keys

On 2/17/25 03:07, Dmitry Vyukov wrote:
>  
>  error:
> +	/*
> +	 * If the application registers rseq, and ever switches to another
> +	 * pkey protection (such that the rseq becomes inaccessible), then
> +	 * any context switch will cause failure here attempting to read/write
> +	 * struct rseq and/or rseq_cs. Since context switches are
> +	 * asynchronous and are outside of the application control
> +	 * (not part of the restricted code scope), we temporarily switch
> +	 * to premissive pkey register to read/write rseq/rseq_cs,
> +	 * similarly to signal delivery accesses to altstack.
> +	 *
> +	 * We don't bother to check if the failure really happened due to
> +	 * pkeys or not, since it does not matter (performance-wise and
> +	 * otherwise).
> +	 *
> +	 * If the restricted code installs rseq_cs in inaccessible to it
> +	 * due to pkeys memory, we still let this function read the rseq_cs.
> +	 * It's unclear what benefits the resticted code gets by doing this
> +	 * (it probably already hijacked control flow at this point), and
> +	 * presumably any sane sandbox should prohibit restricted code
> +	 * from accessing struct rseq, and this is still better than
> +	 * terminating the app unconditionally (it always has a choice
> +	 * of not using rseq and pkeys together).
> +	 */

I would trim this comment down. I'd keep the discussion more in the
changelog than in here. I'd also suggest breaking out the spell checker.

Also, as usual, changing this to imperative voice makes it more compact too:

	Don't bother to check if the failure really happened due to
	pkeys or not, since it does not matter (performance-wise and
	otherwise).

Basically, zap the "we's".

> +	if (!switched_pkey_reg) {
> +		switched_pkey_reg = true;
> +		saved = switch_to_permissive_pkey_reg();
> +		goto retry;
> +	} else {
> +		write_pkey_reg(saved);
> +	}

This code flow is a bit hard to follow with the retry and all.

I think the assumption here is that overwriting the pkey register is too
slow for the fast path. Instead, in the slow error path, there is a
one-time operation to make the register permissive and retry.

I guess it's your rseq code. But I'd probably just put the
switch_to_permissive_pkey_reg()/write_pkey_reg() in the fast/common path
for simplicity unless I knew it was causing a measurable performance
problem.

In either case, it would be great to comment that design choice in the
changelog.

Oh, and cover letters are most appreciated for these kinds of things.
I'd normally reply to the cover letter and say this, but I'll put it
here instead:

The series overall looks fine. It just needs a few cosmetic tweaks.

I don't see any Cc:stable@ or Fixes: tags. Is this a bug fix that you
want backported? If so, those tags would be appropriate and it woudl be
appreciated if you could dig out what it actually fixes.