linux-kernel - Re: [PATCH 1/2] arm64/sve: Flush foreign register state in sve_init

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: <ZypuQNhWHKut8mLl@J2N7QTR9R3.cambridge.arm.com>
Date: Tue, 5 Nov 2024 19:13:04 +0000
From: Mark Rutland <mark.rutland@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] arm64/sve: Flush foreign register state in
 sve_init_regs()

On Wed, Oct 30, 2024 at 08:23:50PM +0000, Mark Brown wrote:
> When we update the in memory register state in sve_init_regs() we neglect
> to flush the task's CPU binding, meaning if the task is rescheduled to
> the last CPU it ran on it is possible for the check for current state in
> fpsimd_thread_switch() to falsely determine that up to date register
> state is present on the CPU.  This results in it incorrectly clearing
> TIF_FOREIGN_FPSTATE and suppress reloading.
> 
> This will also suppress the sve_user_enable() done in
> fpsimd_bind_task_to_cpu() as part of return to userspace, causing
> spurious SVE access traps.
> 
> Call fpsimd_flush_task_state() to invalidate the last loaded CPU
> recorded in the task.
> 
> Fixes: cccb78ce89c4 ("arm64/sve: Rework SVE access trap to convert state in registers")
> Reported-by: Mark Rutlamd <mark.rutland@....com>
> Signed-off-by: Mark Brown <broonie@...nel.org>
> Cc: stable@...r.kernel.org

How about the following:

| arm64/sve: Discard stale CPU state when handling SVE traps
| 
| The logic for handling SVE traps manipulates saved FPSIMD/SVE state
| incorrectly, and a race with preemption can result in a task having
| TIF_SVE set and TIF_FOREIGN_FPSTATE clear even though the live CPU state
| is stale (e.g. with SVE traps enabled). This has been observed to result
| in warnings from do_sve_acc() where SVE traps are not expected while
| TIF_SVE is set:
| 
| |         if (test_and_set_thread_flag(TIF_SVE))
| |                 WARN_ON(1); /* SVE access shouldn't have trapped */
| 
| Warnings of this form have been reported intermittently, e.g.
| 
|   https://lore.kernel.org/linux-arm-kernel/CA+G9fYtEGe_DhY2Ms7+L7NKsLYUomGsgqpdBj+QwDLeSg=JhGg@mail.gmail.com/
|   https://lore.kernel.org/linux-arm-kernel/000000000000511e9a060ce5a45c@google.com/
| 
| The race can occur when the SVE trap handler is preempted before and
| after manipulating the saved FPSIMD/SVE state, starting and ending on
| the same CPU, e.g.
| 
| | void do_sve_acc(unsigned long esr, struct pt_regs *regs)
| | {
| |         // Trap on CPU 0 with TIF_SVE clear, SVE traps enabled
| |         // task->fpsimd_cpu is 0.
| |         // per_cpu_ptr(&fpsimd_last_state, 0) is task.
| | 
| |         ...
| | 
| |         // Preempted; migrated from CPU 0 to CPU 1.
| |         // TIF_FOREIGN_FPSTATE is set.
| | 
| |         get_cpu_fpsimd_context();
| | 
| |         if (test_and_set_thread_flag(TIF_SVE))
| |                 WARN_ON(1); /* SVE access shouldn't have trapped */
| | 
| |         sve_init_regs() {
| |                 if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
| |                         ...
| |                         fpsimd_bind_task_to_cpu();
| |                 } else {
| |                         fpsimd_to_sve(current);
| |                         current->thread.fp_type = FP_STATE_SVE;
| |                 }
| |         }
| | 
| |         put_cpu_fpsimd_context();
| | 
| |         // Preempted; migrated from CPU 1 to CPU 0.
| |         // task->fpsimd_cpu is still 0
| |         // If per_cpu_ptr(&fpsimd_last_state, 0) is still task then:
| |         // - Stale HW state is reused (with SVE traps enabled)
| |         // - TIF_FOREIGN_FPSTATE is cleared
| |         // - A return to userspace skips HW state restore
| | }
| 
| In the case where sve_init_regs() is called while the state is live and
| TIF_FOREIGN_FPSTATE is clear, the state is correctly modified and the
| call to fpsimd_bind_task_to_cpu() disables the SVE trap.
| 
| Fix the case where the state is not live and TIF_FOREIGN_FPSTATE is set
| by calling fpsimd_flush_task_state() to detach from the saved CPU
| state. This ensures that a subsequent context switch will not reuse the
| stale CPU state, and will instead set TIF_FOREIGN_FPSTATE, forcing the
| new state to be reloaded from memory prior to a return to userspace.
| 
| Fixes: cccb78ce89c4 ("arm64/sve: Rework SVE access trap to convert state in registers")
| Reported-by: Mark Rutland <mark.rutland@....com>
| Signed-off-by: Mark Brown <broonie@...nel.org>
| Cc: stable@...r.kernel.org

Note: I fixed the typo (s/Rutlamd/Rutland)

With that:

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

Mark.

> ---
>  arch/arm64/kernel/fpsimd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 77006df20a75aee7c991cf116b6d06bfe953d1a4..6d21971ae5594f32947480cfa168db400a69a283 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1367,6 +1367,7 @@ static void sve_init_regs(void)
>  	} else {
>  		fpsimd_to_sve(current);
>  		current->thread.fp_type = FP_STATE_SVE;
> +		fpsimd_flush_task_state(current);
>  	}
>  }
>  
> 
> -- 
> 2.39.2
> 
>