linux-kernel - Re: [PATCH] x86: Use __fpu_invalidate_fpregs

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Aug 2023 14:56:32 -0700
From:   Sohil Mehta <sohil.mehta@...el.com>
To:     Lijun Pan <lijun.pan@...el.com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <x86@...nel.org>, <hpa@...or.com>, <dave.hansen@...ux.intel.com>,
        <luto@...nel.org>, <peterz@...radead.org>
CC:     <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: Use __fpu_invalidate_fpregs_state() in exec

On 8/18/2023 12:35 PM, Lijun Pan wrote:

> So, I am thinking if it is more rigorous to have both 
> (__cpu_invalidate_fpregs_state, __fpu_invalidate_fpregs_state) 
> invalidated, similarly as fpregs_state_valid checks both conditions.
> 
> code changes like below:
> diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/kernel/fpu/context.h
> index 958accf2ccf0..fd3304bed0a2 100644
> --- a/arch/x86/kernel/fpu/context.h
> +++ b/arch/x86/kernel/fpu/context.h
> @@ -19,8 +19,8 @@
>    * FPU state for a task MUST let the rest of the kernel know that the
>    * FPU registers are no longer valid for this task.
>    *
> - * Either one of these invalidation functions is enough. Invalidate
> - * a resource you control: CPU if using the CPU for something else
> + * To be more rigorous and to prevent from future corner case, Invalidate
> + * both resources you control: CPU if using the CPU for something else
>    * (with preemption disabled), FPU for the current task, or a task that
>    * is prevented from running by the current task.
>    */

This is a general comment in the header to guide multiple scenarios. I
am not sure if invalidating both would be feasible in all scenarios. For
example, in cases such as the fpu_idle_fpregs() there is no task fpu
available.

I think the current issue stems from the fact that the code in exec()
doesn't really control the CPU. It is mostly running with preemption
enabled which can cause the CPUs to change as described in Rick's
scenario. Thus clearing CPU1's FPU pointer in step 4 seems unnecessary.

An extra invalidation probably won't impact performance for such a rare
scenario. However, replacing it with __fpu_invalidate_fpregs_state() to
invalidate the task's fpu state looks correct to me.

Maybe, we can expand the comment above to specify exactly when a
particular invalidation is expected vs when both are expected.

> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 97a899bf98b9..08b9cef0e076 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -725,6 +725,7 @@ static void fpu_reset_fpregs(void)
> 
>          fpregs_lock();
>          fpu__drop(fpu);
> +       __fpu_invalidate_fpregs_state(fpu);
>          /*
>           * This does not change the actual hardware registers. It just
>           * resets the memory image and sets TIF_NEED_FPU_LOAD so a
> 
> Thanks,
> Lijun
>