[PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Kyle Huey]

From: Kyle Huey <me@kylehuey.com>

When management of the PKRU register was moved away from XSTATE, emulation
of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not
for APIs that write XSTATE. This can be seen by running gdb and executing
`p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the
write to the PKRU register (which gdb performs through ptrace) is ignored.

There are three relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE,
sigreturn, and KVM_SET_XSAVE. KVM_SET_XSAVE has its own special handling to
make PKRU writes take effect (in fpu_copy_uabi_to_guest_fpstate). Push that
down into copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE
and sigreturn pass in pointers to the appropriate PKRU value.

This also adds code to initialize the PKRU value to the hardware init value
(namely 0) if the PKRU bit is not set in the XSTATE header to match XRSTOR.
This is a change to the current KVM_SET_XSAVE behavior.

Signed-off-by: Kyle Huey <me@kylehuey.com>
Cc: kvm@vger.kernel.org # For edge case behavior of KVM_SET_XSAVE
Cc: stable@vger.kernel.org # 5.14+
Fixes: e84ba47e313dbc097bf859bb6e4f9219883d5f78
---
 arch/x86/kernel/fpu/core.c   | 11 +----------
 arch/x86/kernel/fpu/regset.c |  2 +-
 arch/x86/kernel/fpu/signal.c |  2 +-
 arch/x86/kernel/fpu/xstate.c | 26 +++++++++++++++++++++-----
 arch/x86/kernel/fpu/xstate.h |  4 ++--
 5 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 0531d6a06df5..dfb79e2ee81f 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -406,16 +406,7 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
 	if (ustate->xsave.header.xfeatures & ~xcr0)
 		return -EINVAL;
 
-	ret = copy_uabi_from_kernel_to_xstate(kstate, ustate);
-	if (ret)
-		return ret;
-
-	/* Retrieve PKRU if not in init state */
-	if (kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU) {
-		xpkru = get_xsave_addr(&kstate->regs.xsave, XFEATURE_PKRU);
-		*vpkru = xpkru->pkru;
-	}
-	return 0;
+	return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);
 }
 EXPORT_SYMBOL_GPL(fpu_copy_uabi_to_guest_fpstate);
 #endif /* CONFIG_KVM */
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 75ffaef8c299..6d056b68f4ed 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -167,7 +167,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	}
 
 	fpu_force_restore(fpu);
-	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf);
+	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf, &target->thread.pkru);
 
 out:
 	vfree(tmpbuf);
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 91d4b6de58ab..558076dbde5b 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -396,7 +396,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
 
 	fpregs = &fpu->fpstate->regs;
 	if (use_xsave() && !fx_only) {
-		if (copy_sigframe_from_user_to_xstate(fpu->fpstate, buf_fx))
+		if (copy_sigframe_from_user_to_xstate(tsk, buf_fx))
 			return false;
 	} else {
 		if (__copy_from_user(&fpregs->fxsave, buf_fx,
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8340156bfd2..1eea7af4afd9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1197,7 +1197,7 @@ static int copy_from_buffer(void *dst, unsigned int offset, unsigned int size,
 
 
 static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
-			       const void __user *ubuf)
+			       const void __user *ubuf, u32 *pkru)
 {
 	struct xregs_state *xsave = &fpstate->regs.xsave;
 	unsigned int offset, size;
@@ -1235,6 +1235,22 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 	for (i = 0; i < XFEATURE_MAX; i++) {
 		mask = BIT_ULL(i);
 
+		if (i == XFEATURE_PKRU) {
+			/*
+			 * Retrieve PKRU if not in init state, otherwise
+			 * initialize it.
+			 */
+			if (hdr.xfeatures & mask) {
+				struct pkru_state xpkru = {0};
+
+				copy_from_buffer(&xpkru, xstate_offsets[i],
+						 sizeof(xpkru), kbuf, ubuf);
+				*pkru = xpkru.pkru;
+			} else {
+				*pkru = 0;
+			}
+		}
+
 		if (hdr.xfeatures & mask) {
 			void *dst = __raw_xsave_addr(xsave, i);
 
@@ -1264,9 +1280,9 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
  * Convert from a ptrace standard-format kernel buffer to kernel XSAVE[S]
  * format and copy to the target thread. Used by ptrace and KVM.
  */
-int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf)
+int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf, u32 *pkru)
 {
-	return copy_uabi_to_xstate(fpstate, kbuf, NULL);
+	return copy_uabi_to_xstate(fpstate, kbuf, NULL, pkru);
 }
 
 /*
@@ -1274,10 +1290,10 @@ int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf)
  * XSAVE[S] format and copy to the target thread. This is called from the
  * sigreturn() and rt_sigreturn() system calls.
  */
-int copy_sigframe_from_user_to_xstate(struct fpstate *fpstate,
+int copy_sigframe_from_user_to_xstate(struct task_struct *tsk,
 				      const void __user *ubuf)
 {
-	return copy_uabi_to_xstate(fpstate, NULL, ubuf);
+	return copy_uabi_to_xstate(tsk->thread.fpu.fpstate, NULL, ubuf, &tsk->thread.pkru);
 }
 
 static bool validate_independent_components(u64 mask)
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 5ad47031383b..a4ecb04d8d64 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -46,8 +46,8 @@ extern void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
 				      u32 pkru_val, enum xstate_copy_mode copy_mode);
 extern void copy_xstate_to_uabi_buf(struct membuf to, struct task_struct *tsk,
 				    enum xstate_copy_mode mode);
-extern int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf);
-extern int copy_sigframe_from_user_to_xstate(struct fpstate *fpstate, const void __user *ubuf);
+extern int copy_uabi_from_kernel_to_xstate(struct fpstate *fpstate, const void *kbuf, u32 *pkru);
+extern int copy_sigframe_from_user_to_xstate(struct task_struct *tsk, const void __user *ubuf);
 
 
 extern void fpu__init_cpu_xstate(void);
-- 
2.37.0

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Ingo Molnar]

* Kyle Huey <me@kylehuey.com> wrote:

> From: Kyle Huey <me@kylehuey.com>
> 
> When management of the PKRU register was moved away from XSTATE, emulation
> of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not
> for APIs that write XSTATE. This can be seen by running gdb and executing
> `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the
> write to the PKRU register (which gdb performs through ptrace) is ignored.
> 
> There are three relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE,
> sigreturn, and KVM_SET_XSAVE. KVM_SET_XSAVE has its own special handling to
> make PKRU writes take effect (in fpu_copy_uabi_to_guest_fpstate). Push that
> down into copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE
> and sigreturn pass in pointers to the appropriate PKRU value.
> 
> This also adds code to initialize the PKRU value to the hardware init value
> (namely 0) if the PKRU bit is not set in the XSTATE header to match XRSTOR.
> This is a change to the current KVM_SET_XSAVE behavior.
> 
> Signed-off-by: Kyle Huey <me@kylehuey.com>
> Cc: kvm@vger.kernel.org # For edge case behavior of KVM_SET_XSAVE
> Cc: stable@vger.kernel.org # 5.14+
> Fixes: e84ba47e313dbc097bf859bb6e4f9219883d5f78
> ---
>  arch/x86/kernel/fpu/core.c   | 11 +----------
>  arch/x86/kernel/fpu/regset.c |  2 +-
>  arch/x86/kernel/fpu/signal.c |  2 +-
>  arch/x86/kernel/fpu/xstate.c | 26 +++++++++++++++++++++-----
>  arch/x86/kernel/fpu/xstate.h |  4 ++--
>  5 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 0531d6a06df5..dfb79e2ee81f 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -406,16 +406,7 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
>  	if (ustate->xsave.header.xfeatures & ~xcr0)
>  		return -EINVAL;
>  
> -	ret = copy_uabi_from_kernel_to_xstate(kstate, ustate);
> -	if (ret)
> -		return ret;
> -
> -	/* Retrieve PKRU if not in init state */
> -	if (kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU) {
> -		xpkru = get_xsave_addr(&kstate->regs.xsave, XFEATURE_PKRU);
> -		*vpkru = xpkru->pkru;
> -	}
> -	return 0;
> +	return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);
>  }
>  EXPORT_SYMBOL_GPL(fpu_copy_uabi_to_guest_fpstate);
>  #endif /* CONFIG_KVM */
> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> index 75ffaef8c299..6d056b68f4ed 100644
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -167,7 +167,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
>  	}
>  
>  	fpu_force_restore(fpu);
> -	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf);
> +	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf, &target->thread.pkru);
>  
>  out:
>  	vfree(tmpbuf);
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 91d4b6de58ab..558076dbde5b 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -396,7 +396,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>  
>  	fpregs = &fpu->fpstate->regs;
>  	if (use_xsave() && !fx_only) {
> -		if (copy_sigframe_from_user_to_xstate(fpu->fpstate, buf_fx))
> +		if (copy_sigframe_from_user_to_xstate(tsk, buf_fx))
>  			return false;
>  	} else {
>  		if (__copy_from_user(&fpregs->fxsave, buf_fx,
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8340156bfd2..1eea7af4afd9 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1197,7 +1197,7 @@ static int copy_from_buffer(void *dst, unsigned int offset, unsigned int size,
>  
>  
>  static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
> -			       const void __user *ubuf)
> +			       const void __user *ubuf, u32 *pkru)
>  {
>  	struct xregs_state *xsave = &fpstate->regs.xsave;
>  	unsigned int offset, size;
> @@ -1235,6 +1235,22 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
>  	for (i = 0; i < XFEATURE_MAX; i++) {
>  		mask = BIT_ULL(i);
>  
> +		if (i == XFEATURE_PKRU) {
> +			/*
> +			 * Retrieve PKRU if not in init state, otherwise
> +			 * initialize it.
> +			 */
> +			if (hdr.xfeatures & mask) {
> +				struct pkru_state xpkru = {0};
> +
> +				copy_from_buffer(&xpkru, xstate_offsets[i],
> +						 sizeof(xpkru), kbuf, ubuf);

Shouldn't the failure case of copy_from_buffer() be handled?

Also, what's the security model for this register, do we trust all input 
values user-space provides for the PKRU field in the XSTATE? I realize that 
WRPKRU already gives user-space write access to the register - but does the 
CPU write it all into the XSTATE, with no restrictions on content 
whatsoever?

Thanks,

	Ingo

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Kyle Huey]

On Wed, Aug 3, 2022 at 2:03 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Kyle Huey <me@kylehuey.com> wrote:
>
> > From: Kyle Huey <me@kylehuey.com>
> >
> > When management of the PKRU register was moved away from XSTATE, emulation
> > of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not
> > for APIs that write XSTATE. This can be seen by running gdb and executing
> > `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the
> > write to the PKRU register (which gdb performs through ptrace) is ignored.
> >
> > There are three relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE,
> > sigreturn, and KVM_SET_XSAVE. KVM_SET_XSAVE has its own special handling to
> > make PKRU writes take effect (in fpu_copy_uabi_to_guest_fpstate). Push that
> > down into copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE
> > and sigreturn pass in pointers to the appropriate PKRU value.
> >
> > This also adds code to initialize the PKRU value to the hardware init value
> > (namely 0) if the PKRU bit is not set in the XSTATE header to match XRSTOR.
> > This is a change to the current KVM_SET_XSAVE behavior.
> >
> > Signed-off-by: Kyle Huey <me@kylehuey.com>
> > Cc: kvm@vger.kernel.org # For edge case behavior of KVM_SET_XSAVE
> > Cc: stable@vger.kernel.org # 5.14+
> > Fixes: e84ba47e313dbc097bf859bb6e4f9219883d5f78
> > ---
> >  arch/x86/kernel/fpu/core.c   | 11 +----------
> >  arch/x86/kernel/fpu/regset.c |  2 +-
> >  arch/x86/kernel/fpu/signal.c |  2 +-
> >  arch/x86/kernel/fpu/xstate.c | 26 +++++++++++++++++++++-----
> >  arch/x86/kernel/fpu/xstate.h |  4 ++--
> >  5 files changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> > index 0531d6a06df5..dfb79e2ee81f 100644
> > --- a/arch/x86/kernel/fpu/core.c
> > +++ b/arch/x86/kernel/fpu/core.c
> > @@ -406,16 +406,7 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
> >       if (ustate->xsave.header.xfeatures & ~xcr0)
> >               return -EINVAL;
> >
> > -     ret = copy_uabi_from_kernel_to_xstate(kstate, ustate);
> > -     if (ret)
> > -             return ret;
> > -
> > -     /* Retrieve PKRU if not in init state */
> > -     if (kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU) {
> > -             xpkru = get_xsave_addr(&kstate->regs.xsave, XFEATURE_PKRU);
> > -             *vpkru = xpkru->pkru;
> > -     }
> > -     return 0;
> > +     return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);
> >  }
> >  EXPORT_SYMBOL_GPL(fpu_copy_uabi_to_guest_fpstate);
> >  #endif /* CONFIG_KVM */
> > diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> > index 75ffaef8c299..6d056b68f4ed 100644
> > --- a/arch/x86/kernel/fpu/regset.c
> > +++ b/arch/x86/kernel/fpu/regset.c
> > @@ -167,7 +167,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
> >       }
> >
> >       fpu_force_restore(fpu);
> > -     ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf);
> > +     ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf, &target->thread.pkru);
> >
> >  out:
> >       vfree(tmpbuf);
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index 91d4b6de58ab..558076dbde5b 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -396,7 +396,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
> >
> >       fpregs = &fpu->fpstate->regs;
> >       if (use_xsave() && !fx_only) {
> > -             if (copy_sigframe_from_user_to_xstate(fpu->fpstate, buf_fx))
> > +             if (copy_sigframe_from_user_to_xstate(tsk, buf_fx))
> >                       return false;
> >       } else {
> >               if (__copy_from_user(&fpregs->fxsave, buf_fx,
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index c8340156bfd2..1eea7af4afd9 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -1197,7 +1197,7 @@ static int copy_from_buffer(void *dst, unsigned int offset, unsigned int size,
> >
> >
> >  static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
> > -                            const void __user *ubuf)
> > +                            const void __user *ubuf, u32 *pkru)
> >  {
> >       struct xregs_state *xsave = &fpstate->regs.xsave;
> >       unsigned int offset, size;
> > @@ -1235,6 +1235,22 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
> >       for (i = 0; i < XFEATURE_MAX; i++) {
> >               mask = BIT_ULL(i);
> >
> > +             if (i == XFEATURE_PKRU) {
> > +                     /*
> > +                      * Retrieve PKRU if not in init state, otherwise
> > +                      * initialize it.
> > +                      */
> > +                     if (hdr.xfeatures & mask) {
> > +                             struct pkru_state xpkru = {0};
> > +
> > +                             copy_from_buffer(&xpkru, xstate_offsets[i],
> > +                                              sizeof(xpkru), kbuf, ubuf);
>
> Shouldn't the failure case of copy_from_buffer() be handled?

Yes, it should be. The sigreturn case could hit it.

> Also, what's the security model for this register, do we trust all input
> values user-space provides for the PKRU field in the XSTATE? I realize that
> WRPKRU already gives user-space write access to the register - but does the
> CPU write it all into the XSTATE, with no restrictions on content
> whatsoever?

There is no security model for this register. The CPU does write
whatever is given to WRPKRU (or XRSTOR) into the PKRU register. The
pkeys(7) man page notes:

Protection keys have the potential to add a layer of security and
reliability to applications. But they have not been primarily designed
as a security feature. For instance, WRPKRU is a completely
unprivileged instruction, so pkeys are useless in any case that an
attacker controls the PKRU register or can execute arbitrary
instructions.

And the ERIM paper
(https://www.usenix.org/system/files/sec19-vahldiek-oberwagner_0.pdf)
explicitly contemplates the need to protect against the less
privileged code containing WRPKRU and XRSTOR instructions (though they
do seem to have missed the implicit XRSTOR in sigreturn).

> Thanks,
>
>         Ingo

- Kyle

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Ingo Molnar]

* Kyle Huey <me@kylehuey.com> wrote:

> > Also, what's the security model for this register, do we trust all 
> > input values user-space provides for the PKRU field in the XSTATE? I 
> > realize that WRPKRU already gives user-space write access to the 
> > register - but does the CPU write it all into the XSTATE, with no 
> > restrictions on content whatsoever?
> 
> There is no security model for this register. The CPU does write whatever 
> is given to WRPKRU (or XRSTOR) into the PKRU register. The pkeys(7) man 
> page notes:
> 
> Protection keys have the potential to add a layer of security and 
> reliability to applications. But they have not been primarily designed as 
> a security feature. For instance, WRPKRU is a completely unprivileged 
> instruction, so pkeys are useless in any case that an attacker controls 
> the PKRU register or can execute arbitrary instructions.

Ok - allowing ptrace to set the full 32 bits of the PKRU register seems OK 
then, and is 100% equivalent to using WRPKRU, right? So there's no implicit 
masking/clearing of bits depending on how many keys are available, or other 
details where WRPKRU might differ from a pure 32-bit per thread write, 
correct?

Thanks,

	Ingo

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Paolo Bonzini]

On 8/3/22 19:25, Ingo Molnar wrote:
> Ok - allowing ptrace to set the full 32 bits of the PKRU register seems OK
> then, and is 100% equivalent to using WRPKRU, right? So there's no implicit
> masking/clearing of bits depending on how many keys are available, or other
> details where WRPKRU might differ from a pure 32-bit per thread write,
> correct?

Yes, it's the same.

Paolo

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Kyle Huey]

On Wed, Aug 3, 2022 at 10:25 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Kyle Huey <me@kylehuey.com> wrote:
>
> > > Also, what's the security model for this register, do we trust all
> > > input values user-space provides for the PKRU field in the XSTATE? I
> > > realize that WRPKRU already gives user-space write access to the
> > > register - but does the CPU write it all into the XSTATE, with no
> > > restrictions on content whatsoever?
> >
> > There is no security model for this register. The CPU does write whatever
> > is given to WRPKRU (or XRSTOR) into the PKRU register. The pkeys(7) man
> > page notes:
> >
> > Protection keys have the potential to add a layer of security and
> > reliability to applications. But they have not been primarily designed as
> > a security feature. For instance, WRPKRU is a completely unprivileged
> > instruction, so pkeys are useless in any case that an attacker controls
> > the PKRU register or can execute arbitrary instructions.
>
> Ok - allowing ptrace to set the full 32 bits of the PKRU register seems OK
> then, and is 100% equivalent to using WRPKRU, right? So there's no implicit
> masking/clearing of bits depending on how many keys are available, or other
> details where WRPKRU might differ from a pure 32-bit per thread write,
> correct?

Right. The hardware doesn't have any concept of what keys are
available or not, that exists entirely in the kernel.

- Kyle

> Thanks,
>
>         Ingo

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace.

[Thorsten Leemhuis]

[Note: this mail is primarily send for documentation purposes and/or for
regzbot, my Linux kernel regression tracking bot. That's why I removed
most or all folks from the list of recipients, but left any that looked
like a mailing lists. These mails usually contain '#forregzbot' in the
subject, to make them easy to spot and filter out.]

[TLDR: I'm adding this regression report to the list of tracked
regressions; all text from me you find below is based on a few templates
paragraphs you might have encountered already already in similar form.]

Hi, this is your Linux kernel regression tracker.

On 31.07.22 07:03, Kyle Huey wrote:
> From: Kyle Huey <me@kylehuey.com>
> 
> When management of the PKRU register was moved away from XSTATE, emulation
> of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not
> for APIs that write XSTATE. This can be seen by running gdb and executing
> `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the
> write to the PKRU register (which gdb performs through ptrace) is ignored.

Seem I missed this one, but apparently it needs tracking.

#regzbot ^introduced e84ba47e313dbc
#regzbot title x86/fpu: emulation of PKRU's existence in XSTATE missing
for APIs that write XSTATE
#regzbot ignore-activity
#regzbot monitor
https://lore.kernel.org/all/20221107063807.81774-1-khuey@kylehuey.com/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

Re: [PATCH] x86/fpu: Allow PKRU to be (once again) written by ptrace. #forregzbot

[Thorsten Leemhuis]

[Note: this mail is primarily send for documentation purposes and/or for
regzbot, my Linux kernel regression tracking bot. That's why I removed
most or all folks from the list of recipients, but left any that looked
like a mailing lists. These mails usually contain '#forregzbot' in the
subject, to make them easy to spot and filter out.]

On 09.11.22 11:23, Thorsten Leemhuis wrote:

> On 31.07.22 07:03, Kyle Huey wrote:
>> From: Kyle Huey <me@kylehuey.com>
>>
>> When management of the PKRU register was moved away from XSTATE, emulation
>> of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not
>> for APIs that write XSTATE. This can be seen by running gdb and executing
>> `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the
>> write to the PKRU register (which gdb performs through ptrace) is ignored.
> 
> Seem I missed this one, but apparently it needs tracking.
> 
> #regzbot ^introduced e84ba47e313dbc
> #regzbot title x86/fpu: emulation of PKRU's existence in XSTATE missing
> for APIs that write XSTATE
> #regzbot ignore-activity
> #regzbot monitor
> https://lore.kernel.org/all/20221107063807.81774-1-khuey@kylehuey.com/

#regzbot fixed-by: 4a804c4f83