arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's

[PATCH] arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's

Posted by Breno Leitao 1 month, 1 week ago

sve_set_common() is the backend for PTRACE_SETREGSET(NT_ARM_SVE) and
PTRACE_SETREGSET(NT_ARM_SSVE). Every write in the function operates on
the tracee (target) - except a single memset that uses current instead,
zeroing the tracer's saved V0-V31 / FPSR / FPCR shadow on every ptrace
SETREGSET call.

The memset is meant to give the tracee a defined zero register image
before the user-supplied payload is copied in (for partial writes,
header-only writes, and FPSIMD<->SVE format switches). Aiming it at
current both denies the tracee that clean slate and silently corrupts
the tracer.

Due to FPSIMD lazy save/restore the wipe only takes effect when the
tracer's CPU FPSIMD binding is dropped after the memset; the next
return to userspace then reloads V0-V31, FPSR and FPCR as zero. No
signal is raised and ptrace() returns success.

Reproducible on an arm64 kernel with SVE: a single-threaded tracer that
loads a known pattern into V0-V31, issues PTRACE_SETREGSET(NT_ARM_SVE)
on a child, and reads V0-V31 back observes them all zeroed within tens
of thousands of iterations when a sibling thread keeps stealing the
FPSIMD CPU binding.

Fixes: 316283f276eb ("arm64/fpsimd: ptrace: Consistently handle partial writes to NT_ARM_(S)SVE")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/arm64/kernel/ptrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index ba5eab23fd900..4d08598e2891d 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -983,8 +983,8 @@ static int sve_set_common(struct task_struct *target,
 	}
 
 	/* Always zero V regs, FPSR, and FPCR */
-	memset(&current->thread.uw.fpsimd_state, 0,
-	       sizeof(current->thread.uw.fpsimd_state));
+	memset(&target->thread.uw.fpsimd_state, 0,
+	       sizeof(target->thread.uw.fpsimd_state));
 
 	/* Registers: FPSIMD-only case */
 

---
base-commit: 9d0d467c3572e93c5faa2e5906a8bbcd70b24efd
change-id: 20260505-fix_ptrace-1bcad595c09e

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH] arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's

Posted by Catalin Marinas 1 month, 1 week ago

On Tue, 05 May 2026 09:02:13 -0700, Breno Leitao wrote:
> sve_set_common() is the backend for PTRACE_SETREGSET(NT_ARM_SVE) and
> PTRACE_SETREGSET(NT_ARM_SSVE). Every write in the function operates on
> the tracee (target) - except a single memset that uses current instead,
> zeroing the tracer's saved V0-V31 / FPSR / FPCR shadow on every ptrace
> SETREGSET call.
> 
> The memset is meant to give the tracee a defined zero register image
> before the user-supplied payload is copied in (for partial writes,
> header-only writes, and FPSIMD<->SVE format switches). Aiming it at
> current both denies the tracee that clean slate and silently corrupts
> the tracer.
> 
> [...]

Applied to arm64 (for-next/fixes). Also fixed the commit log and added
cc stable. Thanks!

[1/1] arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's
      https://git.kernel.org/arm64/c/5cbb61bf4168

Re: [PATCH] arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's

Posted by Mark Rutland 1 month, 1 week ago

Hi Breno,

On Tue, May 05, 2026 at 09:02:13AM -0700, Breno Leitao wrote:
> sve_set_common() is the backend for PTRACE_SETREGSET(NT_ARM_SVE) and
> PTRACE_SETREGSET(NT_ARM_SSVE). Every write in the function operates on
> the tracee (target) - except a single memset that uses current instead,
> zeroing the tracer's saved V0-V31 / FPSR / FPCR shadow on every ptrace
> SETREGSET call.

Sorry about this; this was my bad and definitely needs to be fixed.

> The memset is meant to give the tracee a defined zero register image
> before the user-supplied payload is copied in (for partial writes,
> header-only writes, and FPSIMD<->SVE format switches). Aiming it at
> current both denies the tracee that clean slate and silently corrupts
> the tracer.
> 
> Due to FPSIMD lazy save/restore the wipe only takes effect when the
> tracer's CPU FPSIMD binding is dropped after the memset; the next
> return to userspace then reloads V0-V31, FPSR and FPCR as zero. No
> signal is raised and ptrace() returns success.

You're right that the corruption of the tracer's state is often masked,
but I don't think the last paragraph describes the circumstances
entirely accurately (e.g. if the binding is lost *before* the memset(),
the issue can still occur).

I think it would be better to say:

  The corruption of the tracer's saved FPSIMD state is not always
  observable. Where the tracer's state is live on a CPU, this may reused
  without loading the corrupted state from memory, and will eventually
  be written back over the corrupted state. Where the tracer's state is
  saved in SVE_PT_REGS_SVE format, only the FPSR and FPCR are clobbered,
  and the effective copy of the vectors is in the task's sve_state.

> Reproducible on an arm64 kernel with SVE: a single-threaded tracer that
> loads a known pattern into V0-V31, issues PTRACE_SETREGSET(NT_ARM_SVE)
> on a child, and reads V0-V31 back observes them all zeroed within tens
> of thousands of iterations when a sibling thread keeps stealing the
> FPSIMD CPU binding.
> 
> Fixes: 316283f276eb ("arm64/fpsimd: ptrace: Consistently handle partial writes to NT_ARM_(S)SVE")
> Signed-off-by: Breno Leitao <leitao@debian.org>

This will need to be Cc'd to stable.

With the fixups above (which I assume Catalin or Will can handle):

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/ptrace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index ba5eab23fd900..4d08598e2891d 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -983,8 +983,8 @@ static int sve_set_common(struct task_struct *target,
>  	}
>  
>  	/* Always zero V regs, FPSR, and FPCR */
> -	memset(&current->thread.uw.fpsimd_state, 0,
> -	       sizeof(current->thread.uw.fpsimd_state));
> +	memset(&target->thread.uw.fpsimd_state, 0,
> +	       sizeof(target->thread.uw.fpsimd_state));
>  
>  	/* Registers: FPSIMD-only case */
>  
> 
> ---
> base-commit: 9d0d467c3572e93c5faa2e5906a8bbcd70b24efd
> change-id: 20260505-fix_ptrace-1bcad595c09e
> 
> Best regards,
> --  
> Breno Leitao <leitao@debian.org>
>

Re: [PATCH] arm64/fpsimd: ptrace: zero target's fpsimd_state, not the tracer's

Posted by Breno Leitao 1 month, 1 week ago

Hello Mark,

On Tue, May 05, 2026 at 06:05:25PM +0100, Mark Rutland wrote:
> Hi Breno,
> 
> On Tue, May 05, 2026 at 09:02:13AM -0700, Breno Leitao wrote:
> > sve_set_common() is the backend for PTRACE_SETREGSET(NT_ARM_SVE) and
> > PTRACE_SETREGSET(NT_ARM_SSVE). Every write in the function operates on
> > the tracee (target) - except a single memset that uses current instead,
> > zeroing the tracer's saved V0-V31 / FPSR / FPCR shadow on every ptrace
> > SETREGSET call.
> 
> Sorry about this; this was my bad and definitely needs to be fixed.

No worries at all. While investigating random coredumps in the Meta fleet
with some colleagues, the randomness of the crashes reminded me of a similar
issue I encountered 10 years ago, where floating-point and vector registers
weren't being properly restored at context switch due to a kernel bug,
and created a reproducer in commit 77fad8bfb1d2f ("selftests/powerpc:
Check FP/VEC on exception in TM"). So, this class of bugs is not
special, although *very* hard to debug.

> > Due to FPSIMD lazy save/restore the wipe only takes effect when the
> > tracer's CPU FPSIMD binding is dropped after the memset; the next
> > return to userspace then reloads V0-V31, FPSR and FPCR as zero. No
> > signal is raised and ptrace() returns success.
> 
> You're right that the corruption of the tracer's state is often masked,
> but I don't think the last paragraph describes the circumstances
> entirely accurately (e.g. if the binding is lost *before* the memset(),
> the issue can still occur).
> 
> I think it would be better to say:
> 
>   The corruption of the tracer's saved FPSIMD state is not always
>   observable. Where the tracer's state is live on a CPU, this may reused
>   without loading the corrupted state from memory, and will eventually
>   be written back over the corrupted state. Where the tracer's state is
>   saved in SVE_PT_REGS_SVE format, only the FPSR and FPCR are clobbered,
>   and the effective copy of the vectors is in the task's sve_state.

Ack!

> > Fixes: 316283f276eb ("arm64/fpsimd: ptrace: Consistently handle partial writes to NT_ARM_(S)SVE")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> This will need to be Cc'd to stable.
> 
> With the fixups above (which I assume Catalin or Will can handle):
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thank you for the review. I won't respin it, assuming Catalin or Will
can handle the message rewrite.

Thanks again!
--breno