[RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Waiman Long]

With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.

As only certain class of applications (like Java) requires disabling
speculative store bypass for security purpose, it may not make sense to
allow the TIF_SSBD bit to be inherited across execve() boundary where the
new application may not need SSBD at all and is probably not aware that
SSBD may have been turned on. This may cause an unnecessary performance
loss of up to 20% in some cases.

The arch_setup_new_exec() function is updated to clear the TIF_SSBD
bit unless it has been force-disabled.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 Documentation/userspace-api/spec_ctrl.rst |  3 +++
 arch/x86/kernel/process.c                 | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/userspace-api/spec_ctrl.rst b/Documentation/userspace-api/spec_ctrl.rst
index c4dbe6f..226aed5 100644
--- a/Documentation/userspace-api/spec_ctrl.rst
+++ b/Documentation/userspace-api/spec_ctrl.rst
@@ -55,6 +55,9 @@ is selected by arg2 of :manpage:`prctl(2)` per task. arg3 is used to hand
 in the control value, i.e. either PR_SPEC_ENABLE or PR_SPEC_DISABLE or
 PR_SPEC_FORCE_DISABLE.
 
+When mitigation is enabled, its state will not be inherited on
+:manpage:`execve(2)` unless it is force-disabled.
+
 Common error codes
 ------------------
 ======= =================================================================
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 7d31192..f207f4d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -252,6 +252,16 @@ void arch_setup_new_exec(void)
 	/* If cpuid was previously disabled for this task, re-enable it. */
 	if (test_thread_flag(TIF_NOCPUID))
 		enable_cpuid();
+
+	/*
+	 * Don't inherit TIF_SSBD across exec boundary unless speculative
+	 * store bypass is force-disabled (e.g. seccomp on).
+	 */
+	if (test_thread_flag(TIF_SSBD) &&
+	   !task_spec_ssb_force_disable(current)) {
+		clear_thread_flag(TIF_SSBD);
+		task_clear_spec_ssb_disable(current);
+	}
 }
 
 static inline void switch_to_bitmap(struct thread_struct *prev,
-- 
1.8.3.1

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Andi Kleen]

On Wed, Dec 19, 2018 at 02:09:50PM -0500, Waiman Long wrote:
> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
> 
> As only certain class of applications (like Java) requires disabling
> speculative store bypass for security purpose, it may not make sense to
> allow the TIF_SSBD bit to be inherited across execve() boundary where the
> new application may not need SSBD at all and is probably not aware that
> SSBD may have been turned on. This may cause an unnecessary performance
> loss of up to 20% in some cases.
> 
> The arch_setup_new_exec() function is updated to clear the TIF_SSBD
> bit unless it has been force-disabled.

This makes it impossible to write a wrapper that turns this mode
on for unmodified programs.

Do you have a real use case where this behavior is a problem?

-Andi

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Waiman Long]

On 12/19/2018 02:38 PM, Andi Kleen wrote:
> On Wed, Dec 19, 2018 at 02:09:50PM -0500, Waiman Long wrote:
>> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
>> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
>>
>> As only certain class of applications (like Java) requires disabling
>> speculative store bypass for security purpose, it may not make sense to
>> allow the TIF_SSBD bit to be inherited across execve() boundary where the
>> new application may not need SSBD at all and is probably not aware that
>> SSBD may have been turned on. This may cause an unnecessary performance
>> loss of up to 20% in some cases.
>>
>> The arch_setup_new_exec() function is updated to clear the TIF_SSBD
>> bit unless it has been force-disabled.
> This makes it impossible to write a wrapper that turns this mode
> on for unmodified programs.

You can always force disable SSB. In that case, all the child processes
will have SSBD on.

>
> Do you have a real use case where this behavior is a problem?
>
> -Andi

Yes, we have an enterprise application partner that found that their
application slow down up to 10-20% depending on how their application
was set up. With the slow setup, the application was spawned by Java
processes causing the SSBD bit to stay on when the application was running.

Cheers,
Longman

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Andi Kleen]

> You can always force disable SSB. In that case, all the child processes
> will have SSBD on.

Okay that sounds reasonable, given the below. Thanks.

-Andi

> 
> >
> > Do you have a real use case where this behavior is a problem?
> >
> > -Andi
> 
> Yes, we have an enterprise application partner that found that their
> application slow down up to 10-20% depending on how their application
> was set up. With the slow setup, the application was spawned by Java
> processes causing the SSBD bit to stay on when the application was running.

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Waiman Long]

On 12/19/2018 02:09 PM, Waiman Long wrote:
> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
>
> As only certain class of applications (like Java) requires disabling
> speculative store bypass for security purpose, it may not make sense to
> allow the TIF_SSBD bit to be inherited across execve() boundary where the
> new application may not need SSBD at all and is probably not aware that
> SSBD may have been turned on. This may cause an unnecessary performance
> loss of up to 20% in some cases.
>
> The arch_setup_new_exec() function is updated to clear the TIF_SSBD
> bit unless it has been force-disabled.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  Documentation/userspace-api/spec_ctrl.rst |  3 +++
>  arch/x86/kernel/process.c                 | 10 ++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/Documentation/userspace-api/spec_ctrl.rst b/Documentation/userspace-api/spec_ctrl.rst
> index c4dbe6f..226aed5 100644
> --- a/Documentation/userspace-api/spec_ctrl.rst
> +++ b/Documentation/userspace-api/spec_ctrl.rst
> @@ -55,6 +55,9 @@ is selected by arg2 of :manpage:`prctl(2)` per task. arg3 is used to hand
>  in the control value, i.e. either PR_SPEC_ENABLE or PR_SPEC_DISABLE or
>  PR_SPEC_FORCE_DISABLE.
>  
> +When mitigation is enabled, its state will not be inherited on
> +:manpage:`execve(2)` unless it is force-disabled.
> +
>  Common error codes
>  ------------------
>  ======= =================================================================
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 7d31192..f207f4d 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -252,6 +252,16 @@ void arch_setup_new_exec(void)
>  	/* If cpuid was previously disabled for this task, re-enable it. */
>  	if (test_thread_flag(TIF_NOCPUID))
>  		enable_cpuid();
> +
> +	/*
> +	 * Don't inherit TIF_SSBD across exec boundary unless speculative
> +	 * store bypass is force-disabled (e.g. seccomp on).
> +	 */
> +	if (test_thread_flag(TIF_SSBD) &&
> +	   !task_spec_ssb_force_disable(current)) {
> +		clear_thread_flag(TIF_SSBD);
> +		task_clear_spec_ssb_disable(current);
> +	}
>  }
>  
>  static inline void switch_to_bitmap(struct thread_struct *prev,

Ping! Any comments of objections?

Cheers,
Longman

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Thomas Gleixner]

On Wed, 19 Dec 2018, Waiman Long wrote:

> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
> 
> As only certain class of applications (like Java) requires disabling
> speculative store bypass for security purpose, it may not make sense to
> allow the TIF_SSBD bit to be inherited across execve() boundary where the
> new application may not need SSBD at all and is probably not aware that
> SSBD may have been turned on. This may cause an unnecessary performance
> loss of up to 20% in some cases.

Lot's of MAY's here. Aside of that this fundamentally changes the
behaviour. I'm not really a fan of doing that.

If there are good reasons to have a non-inherited variant, then we rather
introduce that instead of changing the existing semantics without a way for
existing userspace to notice.

Thanks,

	tglx

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Waiman Long]

On 01/11/2019 02:52 PM, Thomas Gleixner wrote:
> On Wed, 19 Dec 2018, Waiman Long wrote:
>
>> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
>> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
>>
>> As only certain class of applications (like Java) requires disabling
>> speculative store bypass for security purpose, it may not make sense to
>> allow the TIF_SSBD bit to be inherited across execve() boundary where the
>> new application may not need SSBD at all and is probably not aware that
>> SSBD may have been turned on. This may cause an unnecessary performance
>> loss of up to 20% in some cases.
> Lot's of MAY's here. Aside of that this fundamentally changes the
> behaviour. I'm not really a fan of doing that.
>
> If there are good reasons to have a non-inherited variant, then we rather
> introduce that instead of changing the existing semantics without a way for
> existing userspace to notice.

I understand your point. How about adding a ",noexec" auxillary option
to the spec_store_bypass_disable command line to activate this new
behavior without changing the default. Will that be acceptable?

Cheers,
Longman

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Thomas Gleixner]

On Mon, 14 Jan 2019, Waiman Long wrote:
> On 01/11/2019 02:52 PM, Thomas Gleixner wrote:
> > On Wed, 19 Dec 2018, Waiman Long wrote:
> >
> >> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
> >> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
> >>
> >> As only certain class of applications (like Java) requires disabling
> >> speculative store bypass for security purpose, it may not make sense to
> >> allow the TIF_SSBD bit to be inherited across execve() boundary where the
> >> new application may not need SSBD at all and is probably not aware that
> >> SSBD may have been turned on. This may cause an unnecessary performance
> >> loss of up to 20% in some cases.
> > Lot's of MAY's here. Aside of that this fundamentally changes the
> > behaviour. I'm not really a fan of doing that.
> >
> > If there are good reasons to have a non-inherited variant, then we rather
> > introduce that instead of changing the existing semantics without a way for
> > existing userspace to notice.
> 
> I understand your point. How about adding a ",noexec" auxillary option
> to the spec_store_bypass_disable command line to activate this new
> behavior without changing the default. Will that be acceptable?

I'd rather have an explicit PR_SPEC_DISABLE_NOEXEC argument for the PRCTL
so you can decide at the application level what kind of behaviour you want.

Thanks,

	tglx

Re: [RFC PATCH] x86/speculation: Don't inherit TIF_SSBD on execve()

[Waiman Long]

On 01/15/2019 04:48 AM, Thomas Gleixner wrote:
> On Mon, 14 Jan 2019, Waiman Long wrote:
>> On 01/11/2019 02:52 PM, Thomas Gleixner wrote:
>>> On Wed, 19 Dec 2018, Waiman Long wrote:
>>>
>>>> With the default SPEC_STORE_BYPASS_SECCOMP/SPEC_STORE_BYPASS_PRCTL mode,
>>>> the TIF_SSBD bit will be inherited when a new task is fork'ed or cloned.
>>>>
>>>> As only certain class of applications (like Java) requires disabling
>>>> speculative store bypass for security purpose, it may not make sense to
>>>> allow the TIF_SSBD bit to be inherited across execve() boundary where the
>>>> new application may not need SSBD at all and is probably not aware that
>>>> SSBD may have been turned on. This may cause an unnecessary performance
>>>> loss of up to 20% in some cases.
>>> Lot's of MAY's here. Aside of that this fundamentally changes the
>>> behaviour. I'm not really a fan of doing that.
>>>
>>> If there are good reasons to have a non-inherited variant, then we rather
>>> introduce that instead of changing the existing semantics without a way for
>>> existing userspace to notice.
>> I understand your point. How about adding a ",noexec" auxillary option
>> to the spec_store_bypass_disable command line to activate this new
>> behavior without changing the default. Will that be acceptable?
> I'd rather have an explicit PR_SPEC_DISABLE_NOEXEC argument for the PRCTL
> so you can decide at the application level what kind of behaviour you want.
>
> Thanks,
>
> 	tglx

Thanks for the advice. Will work on a v2 to be sent out later this week.

Cheers,
Longman