[GR-43835] Add new SpeculativeExecutionBarriers option instead of AllTargets option for SpectrePHTBarriers

Felix Berlakovich · Felix Berlakovich · commit 412f738e496f · 2023-01-31T17:36:51.000Z
PullRequest: graal/13621
diff --git a/compiler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/SpectrePHTMitigations.java b/compiler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/SpectrePHTMitigations.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -24,11 +24,14 @@
  */
 package org.graalvm.compiler.core.common;
 
+import org.graalvm.collections.EconomicMap;
+import org.graalvm.collections.UnmodifiableEconomicMap;
 import org.graalvm.compiler.options.EnumOptionKey;
 import org.graalvm.compiler.options.Option;
 import org.graalvm.compiler.options.OptionKey;
 import org.graalvm.compiler.options.OptionStability;
 import org.graalvm.compiler.options.OptionType;
+import org.graalvm.compiler.options.OptionValues;
 
 public enum SpectrePHTMitigations {
     None,
@@ -38,8 +41,55 @@ public enum SpectrePHTMitigations {
 
     public static class Options {
         // @formatter:off
+
+        @Option(help = "Stop speculative execution on all branch targets with execution barrier instructions.", stability = OptionStability.STABLE)
+        public static final OptionKey<Boolean> SpeculativeExecutionBarriers = new OptionKey<>(false) {
+
+            @Override
+            public Boolean getValue(OptionValues values) {
+                // Do not use getValue to avoid an infinite recursion
+                if (values.getMap().get(SpectrePHTBarriers) == AllTargets) {
+                    return true;
+                }
+                return super.getValue(values);
+            }
+
+            protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean oldValue, Boolean newValue) {
+                if (values.containsKey(SpectrePHTBarriers)) {
+                    Object otherValue = values.get(SpectrePHTBarriers);
+                    if (newValue && otherValue != AllTargets || (!newValue && otherValue == AllTargets)) {
+                        throw new IllegalArgumentException("SpectrePHTBarriers can be set to 'AllTargets' if and only if SpeculativeExecutionBarriers is enabled or unspecified.");
+                    }
+                }
+            }
+        };
+
         @Option(help = "file:doc-files/MitigateSpeculativeExecutionAttacksHelp.txt")
-        public static final EnumOptionKey<SpectrePHTMitigations> SpectrePHTBarriers = new EnumOptionKey<>(None);
+        public static final EnumOptionKey<SpectrePHTMitigations> SpectrePHTBarriers = new EnumOptionKey<>(None) {
+            private boolean isSpeculativeExecutionBarriersEnabled(UnmodifiableEconomicMap<OptionKey<?>, Object> values) {
+                Object value = values.get(SpeculativeExecutionBarriers);
+                return value != null && (boolean) value;
+            }
+
+            @Override
+            public SpectrePHTMitigations getValue(OptionValues values) {
+                if (isSpeculativeExecutionBarriersEnabled(values.getMap())) {
+                    return AllTargets;
+                }
+                return super.getValue(values);
+            }
+
+            @Override
+            protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, SpectrePHTMitigations oldValue, SpectrePHTMitigations newValue) {
+                if (values.containsKey(SpeculativeExecutionBarriers)) {
+                    boolean otherIsEnabled = isSpeculativeExecutionBarriersEnabled(values);
+                    if (otherIsEnabled && newValue != AllTargets || (!otherIsEnabled && newValue == AllTargets)) {
+                        throw new IllegalArgumentException("SpectrePHTBarriers can be set to 'AllTargets' if and only if SpeculativeExecutionBarriers is enabled or unspecified.");
+                    }
+                }
+            }
+        };
+
         @Option(help = "Mask indices to scope access to allocation size after bounds check.", type = OptionType.User, stability = OptionStability.STABLE)
         public static final OptionKey<Boolean> SpectrePHTIndexMasking = new OptionKey<>(false);
         // @formatter:on
diff --git a/compiler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/doc-files/MitigateSpeculativeExecutionAttacksHelp.txt b/compiler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/doc-files/MitigateSpeculativeExecutionAttacksHelp.txt
@@ -2,8 +2,9 @@ Select a strategy to mitigate speculative bounds check bypass (aka Spectre-PHT o
 This is an experimental option - execution of untrusted code is not supported by GraalVM CE.
 The accepted values are:
                   None - No mitigations are used in JIT compiled code.
-            AllTargets - Speculative execution on all conditional branch targets is
+            AllTargets - Speculative execution on all branch targets is
                          stopped using speculative execution barrier instructions.
+                         This option is equivalent to setting SpeculativeExecutionBarriers to true.
           GuardTargets - Branch targets relevant to Java memory safety are instrumented
                          with barrier instructions. This option has less performance impact
                          than AllTargets. 
diff --git a/compiler/src/org.graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/SpectreFenceTest.java b/compiler/src/org.graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/SpectreFenceTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,13 +25,19 @@
 
 package org.graalvm.compiler.core.test;
 
+import static org.graalvm.compiler.core.common.SpectrePHTMitigations.AllTargets;
+import static org.graalvm.compiler.core.common.SpectrePHTMitigations.GuardTargets;
+import static org.graalvm.compiler.core.common.SpectrePHTMitigations.Options.SpectrePHTBarriers;
+import static org.graalvm.compiler.core.common.SpectrePHTMitigations.Options.SpeculativeExecutionBarriers;
 import static org.junit.Assume.assumeTrue;
 
+import org.graalvm.collections.EconomicMap;
 import org.graalvm.compiler.api.directives.GraalDirectives;
 import org.graalvm.compiler.api.test.Graal;
 import org.graalvm.compiler.core.common.SpectrePHTMitigations;
 import org.graalvm.compiler.nodes.AbstractBeginNode;
 import org.graalvm.compiler.nodes.StructuredGraph;
+import org.graalvm.compiler.options.OptionKey;
 import org.graalvm.compiler.options.OptionValues;
 import org.graalvm.compiler.runtime.RuntimeProvider;
 import org.junit.Assert;
@@ -128,4 +134,38 @@ public void test03() {
         test(getFenceOptions(), "test3Snippet", 10D);
         assertNumberOfFences("test3Snippet", 1);
     }
+
+    @Test
+    public void testOptionConsistency() throws Exception {
+        // If one side is unspecified, the options are synchronized
+        OptionValues onlyExecutionBarriers = new OptionValues(getInitialOptions(), SpeculativeExecutionBarriers, true);
+        Assert.assertTrue(SpeculativeExecutionBarriers.getValue(onlyExecutionBarriers));
+        Assert.assertEquals(SpectrePHTBarriers.getValue(onlyExecutionBarriers), AllTargets);
+        OptionValues onlyAllTargets = new OptionValues(getInitialOptions(), SpectrePHTMitigations.Options.SpectrePHTBarriers, AllTargets);
+        Assert.assertTrue(SpeculativeExecutionBarriers.getValue(onlyAllTargets));
+        Assert.assertEquals(SpectrePHTBarriers.getValue(onlyAllTargets), AllTargets);
+
+        // If both sides are set explicitly, they have to be consistent
+        try {
+            EconomicMap<OptionKey<?>, Object> optionValues = OptionValues.asMap(SpeculativeExecutionBarriers, false);
+            SpectrePHTBarriers.update(optionValues, AllTargets);
+            Assert.fail("Inconsistent option values are not prevented");
+        } catch (IllegalArgumentException e) {
+            // this should happen
+        }
+
+        try {
+            // If both sides are set explicitly, they have to be consistent
+            EconomicMap<OptionKey<?>, Object> optionValues = OptionValues.asMap(SpectrePHTBarriers, GuardTargets);
+            SpeculativeExecutionBarriers.update(optionValues, true);
+            Assert.fail("Inconsistent option values are not prevented");
+        } catch (IllegalArgumentException e) {
+            // this should happen
+        }
+
+        // Explicitly but consistent is fine
+        EconomicMap<OptionKey<?>, Object> optionValues = OptionValues.asMap(SpectrePHTBarriers, AllTargets);
+        SpeculativeExecutionBarriers.update(optionValues, true);
+
+    }
 }
diff --git a/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/gen/NodeLIRBuilder.java b/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/gen/NodeLIRBuilder.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,8 +28,7 @@
 import static jdk.vm.ci.code.ValueUtil.isLegal;
 import static jdk.vm.ci.code.ValueUtil.isRegister;
 import static org.graalvm.compiler.core.common.GraalOptions.MatchExpressions;
-import static org.graalvm.compiler.core.common.SpectrePHTMitigations.AllTargets;
-import static org.graalvm.compiler.core.common.SpectrePHTMitigations.Options.SpectrePHTBarriers;
+import static org.graalvm.compiler.core.common.SpectrePHTMitigations.Options.SpeculativeExecutionBarriers;
 import static org.graalvm.compiler.core.match.ComplexMatchValue.INTERIOR_MATCH;
 import static org.graalvm.compiler.debug.DebugOptions.LogVerbose;
 import static org.graalvm.compiler.lir.LIR.verifyBlock;
@@ -102,8 +101,8 @@
 import org.graalvm.compiler.nodes.calc.IntegerDivRemNode;
 import org.graalvm.compiler.nodes.calc.IntegerTestNode;
 import org.graalvm.compiler.nodes.calc.IsNullNode;
-import org.graalvm.compiler.nodes.cfg.HIRBlock;
 import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
+import org.graalvm.compiler.nodes.cfg.HIRBlock;
 import org.graalvm.compiler.nodes.extended.ForeignCall;
 import org.graalvm.compiler.nodes.extended.IntegerSwitchNode;
 import org.graalvm.compiler.nodes.extended.OpaqueLogicNode;
@@ -346,7 +345,7 @@ private Value[] createPhiOut(AbstractMergeNode merge, AbstractEndNode pred) {
 
     public void doBlockPrologue(@SuppressWarnings("unused") HIRBlock block, @SuppressWarnings("unused") OptionValues options) {
 
-        if (SpectrePHTBarriers.getValue(options) == AllTargets) {
+        if (SpeculativeExecutionBarriers.getValue(options)) {
             boolean hasControlSplitPredecessor = false;
             for (int i = 0; i < block.getPredecessorCount(); i++) {
                 HIRBlock b = block.getPredecessorAt(i);
