Received-SPF: pass (google.com: domain of linux-kernel+bounces-210611-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240606224035.3238985-2-aruna.ramakrishna@oracle.com>
 <20240610213122.3377462-1-jeffxu@chromium.org> <2BA1926A-8720-4B85-8D15-557CB9A8464E@oracle.com>
In-Reply-To: <2BA1926A-8720-4B85-8D15-557CB9A8464E@oracle.com>
From: Jeff Xu <jeffxu@chromium.org>
Date: Tue, 11 Jun 2024 14:26:15 -0700
Message-ID: <CABi2SkUwx=aooM3FnYsOXmNmWz3RNkAJ4TA7C6FWdgsMaBjTag@mail.gmail.com>
Subject: Re: Re [PATCH v5 1/5] x86/pkeys: Add PKRU as a parameter in signal
 handling functions
To: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Cc: "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>, Keith Lucas <keith.lucas@oracle.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>, 
	Thomas Gleixner <tglx@linutronix.de>, "x86@kernel.org" <x86@kernel.org>, 
	Andrew Brownsword <andrew.brownsword@oracle.com>, 
	Matthias Neugschwandtner <matthias.neugschwandtner@oracle.com>, 
	"jeffxu@google.com" <jeffxu@google.com>, "jannh@google.com" <jannh@google.com>, 
	"keescook@chromium.org" <keescook@chromium.org>, "sroettger@google.com" <sroettger@google.com>, 
	"jorgelo@chromium.org" <jorgelo@chromium.org>, 
	"rick.p.edgecombe@intel.com" <rick.p.edgecombe@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 11, 2024 at 6:56=E2=80=AFAM Aruna Ramakrishna
<aruna.ramakrishna@oracle.com> wrote:
>
>
>
> > On Jun 10, 2024, at 2:31=E2=80=AFPM, jeffxu@chromium.org wrote:
> >
> > On Thu, Jun 06, 2024 at 10:40:31PM +0000, Aruna Ramakrishna wrote:
> >
> >> diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm=
/sighandling.h
> >> index e770c4fc47f4..de458354a3ea 100644
> >> --- a/arch/x86/include/asm/sighandling.h
> >> +++ b/arch/x86/include/asm/sighandling.h
> >> @@ -17,11 +17,11 @@ void signal_fault(struct pt_regs *regs, void __use=
r *frame, char *where);
> >>
> >> void __user *
> >> get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_=
size,
> >> -     void __user **fpstate);
> >> +     void __user **fpstate, u32 pkru);
> >>
> >> -int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs);
> >> -int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> >> -int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> >> -int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> >> +int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs, u32 =
pkru);
> >> +int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u=
32 pkru);
> >> +int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u3=
2 pkru);
> >> +int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u3=
2 pkru);
> >
> > Does ia32 support PKEY ? I thought it is X64 only.
> > It might be possible to not to change any of ia32 code.
>
> From the Intel manual, it did not seem that pkey support was x64 only.
>
https://docs.kernel.org/core-api/protection-keys.html#memory-protection-key=
s

"The feature is only available in 64-bit mode, even though there is
theoretically space in the PAE PTEs."

> >
> >
> >>
> >> #endif /* _ASM_X86_SIGHANDLING_H */
> >> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal=
.c
> >> index 247f2225aa9f..2b3b9e140dd4 100644
> >> --- a/arch/x86/kernel/fpu/signal.c
> >> +++ b/arch/x86/kernel/fpu/signal.c
> >> @@ -156,7 +156,7 @@ static inline bool save_xstate_epilog(void __user =
*buf, int ia32_frame,
> >> return !err;
> >> }
> >>
> >> -static inline int copy_fpregs_to_sigframe(struct xregs_state __user *=
buf)
> >> +static inline int copy_fpregs_to_sigframe(struct xregs_state __user *=
buf, u32 pkru)
> >> {
> >> if (use_xsave())
> >> return xsave_to_user_sigframe(buf);
> >> @@ -185,7 +185,7 @@ static inline int copy_fpregs_to_sigframe(struct x=
regs_state __user *buf)
> >>  * For [f]xsave state, update the SW reserved fields in the [f]xsave f=
rame
> >>  * indicating the absence/presence of the extended state to the user.
> >>  */
> >> -bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, =
int size)
> >> +bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, =
int size, u32 pkru)
> >> {
> >> struct task_struct *tsk =3D current;
> >> struct fpstate *fpstate =3D tsk->thread.fpu.fpstate;
> >> @@ -228,7 +228,7 @@ bool copy_fpstate_to_sigframe(void __user *buf, vo=
id __user *buf_fx, int size)
> >> fpregs_restore_userregs();
> >>
> >> pagefault_disable();
> >> - ret =3D copy_fpregs_to_sigframe(buf_fx);
> >> + ret =3D copy_fpregs_to_sigframe(buf_fx, pkru);
> >> pagefault_enable();
> >> fpregs_unlock();
> >>
> >> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> >> index 31b6f5dddfc2..94b894437327 100644
> >> --- a/arch/x86/kernel/signal.c
> >> +++ b/arch/x86/kernel/signal.c
> >> @@ -74,7 +74,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
> >>  */
> >> void __user *
> >> get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_=
size,
> >> -     void __user **fpstate)
> >> +     void __user **fpstate, u32 pkru)
> >> {
> >> struct k_sigaction *ka =3D &ksig->ka;
> >> int ia32_frame =3D is_ia32_frame(ksig);
> >> @@ -139,7 +139,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs =
*regs, size_t frame_size,
> >> }
> >>
> >> /* save i387 and extended state */
> >> - if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_=
size))
> >> + if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_=
size, pkru))
> >
> > You might find that we only need to update PKRU right before copy_fpsta=
te_to_sigframe,
> > so no need to pass pkru all the way from top.
> >
>
> The PKRU is updated in copy_fpregs_to_sigframe(), right after xsave_to_us=
er_sigframe().
> But the code that decides or cares about what pkru is written to the sigf=
rame is handle_signal(), so
> it seemed cleaner to do those checks there and pass in pkru as a paramete=
r down the stack.
>
IIUC,  the kernel need the read/write access from
get_sigframe (arch/x86/kernel/signal.c),  it is possible to have the
PKRU check/update PKRU inside get_sigframe, and there is no need to
pass orig_pkru from handle_signal().

Thanks
-Jeff