config: Add DisableSpeculationMitigations #1047

KentaTada · 2020-05-28T04:40:30Z

It disables speculative execution mitigations in the container.
For more information about that, please refer to:
opencontainers/runc#2430

Signed-off-by: Kenta Tada Kenta.Tada@sony.com

giuseppe

just a nit, otherwise LGTM

giuseppe · 2020-05-28T07:21:40Z

specs-go/config.go

@@ -58,6 +58,8 @@ type Process struct {
 	OOMScoreAdj *int `json:"oomScoreAdj,omitempty" platform:"linux"`
 	// SelinuxLabel specifies the selinux context that the container process is run as.
 	SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"`
+	// AllowSpeculation disables spectre mitigations


could we just make it more generic like using speculative execution instead of spectre?

I agree with you. I changed the above and config.md and the commit message.

mrunalp · 2020-05-28T18:40:51Z

config.md

@@ -208,6 +208,7 @@ For Linux-based systems, the `process` object supports the following process-spe
    For more information on how these two settings work together, see [the memory cgroup documentation section 10. OOM Contol][cgroup-v1-memory_2].
 * **`selinuxLabel`** (string, OPTIONAL) specifies the SELinux label for the process.
    For more information about SELinux, see  [SELinux documentation][selinux].
+* **`allowSpeculation`** (bool, OPTIONAL) setting `allowSpeculation` to true disable speculative execution mitigations to improve the performance.


I think we should name this disableSpeculationMitigations instead.

I think we should name this disableSpeculationMitigations instead.

disableSpeculationMitigations is easy to understand although I wanted to use the positive boolean variable name. I don't mind whichever it is.
@giuseppe, Could you give me your opinion on the variable name?

I think the name @mrunalp suggested is clearer, since it is about allowing or disabling mitigations not the speculative execution itself.

Thanks! I'll modify it.

@mrunalp @giuseppe I changed the name.

cyphar

While I understand adding a single boolean field is simpler, I'm not sure this is the best approach.

cyphar · 2020-06-01T07:08:39Z

config.md

@@ -208,6 +208,7 @@ For Linux-based systems, the `process` object supports the following process-spe
    For more information on how these two settings work together, see [the memory cgroup documentation section 10. OOM Contol][cgroup-v1-memory_2].
 * **`selinuxLabel`** (string, OPTIONAL) specifies the SELinux label for the process.
    For more information about SELinux, see  [SELinux documentation][selinux].
+* **`disableSpeculationMitigations`** (bool, OPTIONAL) setting `disableSpeculationMitigations` to true disable speculative execution mitigations to improve the performance.


I think this needs more explanation as an option (and should be reworded to not repeat the name of the setting), and preferably a link to the kernel documentation for speculation mitigations.

Suggested change

* **`disableSpeculationMitigations`** (bool, OPTIONAL) setting `disableSpeculationMitigations` to true disable speculative execution mitigations to improve the performance.

* **`disableSpeculationMitigations`** (bool, OPTIONAL) specifies whether CPU speculative execution mitigations should be disabled for the process. Several mitigations are auto-enabled under Linux, and can cause a noticeable performance impact (depending on your workload). Note that enabling this option may reduce the security properties of containers created with this configuration. See [the kernel documentation][speculative-control] for more information.

[speculative-control]: https://www.kernel.org/doc/html/latest/userspace-api/spec_ctrl.html

However, looking at the kernel documentation, i notice that this new configuration option doesn't allow users to specify PR_SPEC_FORCE_DISABLE -- which forces a container to have speculation mitigations enabled. There's also an argument that users should be able to specify which mitigations to disable... Maybe we should instead have a mitigations object that has more than one option:

"speculationMitigations": { "defaultRule": "force-enable", "exceptions": [ { "mitigation": "store-bypass", "rule": "disable", }, ], }

The above configuration would result in PR_SPEC_STORE_BYPASS being set to PR_SPEC_ENABLE, and PR_SPEC_INDIR_BRANCH being set to PR_SPEC_FORCE_DISABLE.

I think this needs more explanation as an option (and should be reworded to not repeat the name of the setting), and preferably a link to the kernel documentation for speculation mitigations.

I completely agree with you. I should prepare for more explanation. Thanks!

Maybe we should instead have a mitigations object that has more than one option:

I agree to use object but please let me explain the background of prctl and seccomp.

The kernel automatically forces a container to have all speculation mitigations enabled when seccomp is used without SECCOMP_FILTER_FLAG_SPEC_ALLOW on x86_64.
https://github.com/torvalds/linux/blob/v5.7-rc7/arch/x86/kernel/cpu/bugs.c#L1201
It affects the result of prctl.

In addition to that, I found the kernel bug that the kernel only forced to enable Speculative Store Bypass mitigations and not to enable Indirect Branch Speculation mitigations even if seccomp was used. I may create a kernel patch after more investigation.

Anyway, I thought it was difficult for users to set up complex settings at first.
But I reconsider and it is no problem if container runtime handles it appropriately.

I've just realized we already have SECCOMP_FILTER_FLAG_SPEC_ALLOW in the seccomp flags (and I've added it): #1018

@giuseppe @cyphar

I've just realized we already have SECCOMP_FILTER_FLAG_SPEC_ALLOW in the seccomp flags (and I've added it): #1018

SECCOMP_FILTER_FLAG_SPEC_ALLOW is also needed to disable mitigations when secccomp is used as mentioned previously.
I think we will implement the container runtime using secomp with SECCOMP_FILTER_FLAG_SPEC_ALLOW + prctl like below:
https://chromium.googlesource.com/chromium/src/sandbox/+/be48c498815b50c9585332d35bb5f6f4920c41de

But

However, looking at the kernel documentation, i notice that this new configuration option doesn't allow users to specify PR_SPEC_FORCE_DISABLE -- which forces a container to have speculation mitigations enabled. There's also an argument that users should be able to specify which mitigations to disable... Maybe we should instead have a mitigations object that has more than one option:

I don't know the reason but https://www.kernel.org/doc/html/latest/userspace-api/spec_ctrl.html is different from the actual behavior.

When PR_SPEC_FORCE_DISABLE is set for PR_SPEC_STORE_BYPASS, a subsequent prctl(…, PR_SPEC_ENABLE) will fail. This is as same as the kernel document.

When PR_SPEC_FORCE_DISABLE is set for PR_SPEC_INDIRECT_BRANCH, a subsequent prctl(…, PR_SPEC_ENABLE) will not fail as below comments from kernel community. This is not as same as the kernel document.
https://lore.kernel.org/patchwork/patch/1251849/

Actually, my below modified runc can disable IBPB/STIBP mitigation even if seccomp without SECCOMP_FILTER_FLAG_SPEC_ALLOW is enabled.
opencontainers/runc#2433

I'm sorry but please give me some time to reconsider this interface because it is complicated. And please give me feedback.
Thanks!

FYI:

When PR_SPEC_FORCE_DISABLE is set for PR_SPEC_INDIRECT_BRANCH, a subsequent prctl(…, PR_SPEC_ENABLE) will not fail as below comments from kernel community. This is not as same as the kernel document.
https://lore.kernel.org/patchwork/patch/1251849/

After my patch, another person modified this issue as same as my patch.
https://lore.kernel.org/patchwork/patch/1253799/

So, the complicated specification or bug is fixed. I'll reconsider the interface.

@cyphar I apologize for the late reply. I updated to use object.
Could you review the latest commit?

@cyphar PTAL ^^^

It disables speculative execution mitigations in the container. For more information about that, please refer to: opencontainers/runc#2430 Co-Authored-By: Aleksa Sarai <cyphar@cyphar.com> Signed-off-by: Kenta Tada <Kenta.Tada@sony.com>

KentaTada · 2021-07-20T23:00:32Z

@cyphar
This feature is also needed in moby/moby#42619
Could you review my update?

kailun-qin · 2021-08-04T03:31:47Z

config.md

+            * `enable` - The mitigation of this particular speculation is disabled.
+            * `disable` - The mitigation of this particular speculation is enabled.
+            * `force-disable` - Same as disable, but it cannot be undone.
+            * `disable-noexec` - Same as disable, but the state will be cleared on execve(2).


disable-noexec is not applicable/supported for indirect-branch. Maybe we need a note here?

kailun-qin · 2021-08-04T03:35:13Z

specs-go/config.go

+}
+
+// SpecExceptions is used to specify the setting of speculative execution mitigations.
+type SpecExceptions struct {


SpecException instead of SpecExceptions, as each defines one exception?

kailun-qin · 2021-08-04T03:41:49Z

config.md

+            * `store-bypass` - Speculative Store Bypass
+            * `indirect-branch` - Indirect Branch Speculation in User Processes
+        * **`rule`** *(string, REQUIRED)* - enables or disables the specific mitigation.
+            * `enable` - The mitigation of this particular speculation is disabled.


Should the rules under exceptions keep the same semantics with disable, or it should reflect/align with the corresponding mitigation's?

kailun-qin · 2021-08-04T08:17:20Z

config.md

+        * `disable` - The mitigation of speculations without `exceptions` is enabled.
+        * `force-disable` - Same as disable, but it cannot be undone.
+        * `disable-noexec` - Same as disable, but the state will be cleared on execve(2).
+    * **`exceptions`** *(array of objects, OPTIONAL)* - the configuration of specific mitigations.


Is there any possibility that we need multiple exceptions with the same mitigation type? If not, what about using dedicate structs with rules for store-bypass and indirect-branch?

KentaTada mentioned this pull request May 28, 2020

Add --allow-speculation option to disable IBPB/STIBP mitigation opencontainers/runc#2433

Draft

giuseppe approved these changes May 28, 2020

View reviewed changes

KentaTada force-pushed the allow-speculation branch from b24d646 to 27747c5 Compare May 28, 2020 08:39

mrunalp reviewed May 28, 2020

View reviewed changes

KentaTada force-pushed the allow-speculation branch from 27747c5 to df66750 Compare June 1, 2020 03:41

KentaTada changed the title ~~config: Add AllowSpeculation~~ config: Add DisableSpeculationMitigations Jun 1, 2020

cyphar requested changes Jun 1, 2020

View reviewed changes

KentaTada force-pushed the allow-speculation branch 2 times, most recently from 86079d3 to eb7ab28 Compare June 1, 2020 10:33

KentaTada force-pushed the allow-speculation branch from eb7ab28 to ea44b36 Compare July 21, 2020 03:13

config: Add DisableSpeculationMitigations

7ce4d0a

It disables speculative execution mitigations in the container. For more information about that, please refer to: opencontainers/runc#2430 Co-Authored-By: Aleksa Sarai <cyphar@cyphar.com> Signed-off-by: Kenta Tada <Kenta.Tada@sony.com>

KentaTada force-pushed the allow-speculation branch from ea44b36 to 7ce4d0a Compare July 21, 2020 03:15

h-vetinari mentioned this pull request Aug 28, 2020

Prepare / Tag v1.1.0 release #1052

Closed

KentaTada mentioned this pull request Jul 20, 2021

support seccomp flags such as SECCOMP_FILTER_FLAG_SPEC_ALLOW (OCI Runtime Spec v1.0.2) opencontainers/runc#2430

Closed

KentaTada mentioned this pull request Jul 20, 2021

Feature request: ability to set SECCOMP_FILTER_FLAG_SPEC_ALLOW in seccomp profiles moby/moby#42619

Closed

kailun-qin reviewed Aug 4, 2021

View reviewed changes

bobbypage mentioned this pull request Feb 2, 2023

KEP-2413: Graduate SeccompDefault to stable kubernetes/enhancements#3718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: Add DisableSpeculationMitigations #1047

config: Add DisableSpeculationMitigations #1047

KentaTada commented May 28, 2020 •

edited

Loading

giuseppe left a comment

giuseppe May 28, 2020

KentaTada May 28, 2020

giuseppe May 28, 2020

mrunalp May 28, 2020

KentaTada May 28, 2020

giuseppe May 29, 2020

KentaTada Jun 1, 2020

KentaTada Jun 1, 2020

cyphar left a comment

cyphar Jun 1, 2020 •

edited

Loading

KentaTada Jun 1, 2020

giuseppe Jun 4, 2020

KentaTada Jun 9, 2020

KentaTada Jun 17, 2020 •

edited

Loading

KentaTada Jul 21, 2020

kolyshkin Feb 22, 2022

KentaTada commented Jul 20, 2021

kailun-qin Aug 4, 2021

kailun-qin Aug 4, 2021

kailun-qin Aug 4, 2021

kailun-qin Aug 4, 2021

-* **`disableSpeculationMitigations`** (bool, OPTIONAL) setting `disableSpeculationMitigations` to true disable speculative execution mitigations to improve the performance.
+* **`disableSpeculationMitigations`** (bool, OPTIONAL) specifies whether CPU speculative execution mitigations should be disabled for the process. Several mitigations are auto-enabled under Linux, and can cause a noticeable performance impact (depending on your workload). Note that enabling this option may reduce the security properties of containers created with this configuration. See [the kernel documentation][speculative-control] for more information.
+[speculative-control]: https://www.kernel.org/doc/html/latest/userspace-api/spec_ctrl.html

config: Add DisableSpeculationMitigations #1047

Are you sure you want to change the base?

config: Add DisableSpeculationMitigations #1047

Conversation

KentaTada commented May 28, 2020 • edited Loading

giuseppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

cyphar Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentaTada Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentaTada commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentaTada commented May 28, 2020 •

edited

Loading

cyphar Jun 1, 2020 •

edited

Loading

KentaTada Jun 17, 2020 •

edited

Loading