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: <CACT4Y+YmdnTznWw5LO4gXHbxbXNPj0jqOO3KG=dJWKEcVbrkvA@mail.gmail.com>
Date: Mon, 24 Feb 2025 14:13:28 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: 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, "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

On Fri, 21 Feb 2025 at 17:26, Dave Hansen <dave.hansen@...el.com> wrote:
>
> ...
> > -/*
> > - * 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();
> }

Makes sense. Done in v3.