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]
Message-ID: <96d2a7b5-bb7a-01e7-fab0-d4874818dc64@intel.com>
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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ