ART: Reject PackedSwitch with overflowing keys

As stated in "Dalvik bytecode" sections on switch payload format,
switch case keys must be stored in ascending order. Verifier enforced
this for sparse-switch but not for packed-switch.

Bug: 24399945
Change-Id: I0802d38e2bfae93c0dffe8ebfce2e9693a63ec02

diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 317a144..9e1a31a 100644 (file)
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -1300,17 +1300,17 @@ bool MethodVerifier::CheckSwitchTargets(uint32_t cur_offset) {
     return false;
   }
 
+  bool is_packed_switch = (*insns & 0xff) == Instruction::PACKED_SWITCH;
+
   uint32_t switch_count = switch_insns[1];
-  int32_t keys_offset, targets_offset;
+  int32_t targets_offset;
   uint16_t expected_signature;
-  if ((*insns & 0xff) == Instruction::PACKED_SWITCH) {
+  if (is_packed_switch) {
     /* 0=sig, 1=count, 2/3=firstKey */
     targets_offset = 4;
-    keys_offset = -1;
     expected_signature = Instruction::kPackedSwitchSignature;
   } else {
     /* 0=sig, 1=count, 2..count*2 = keys */
-    keys_offset = 2;
     targets_offset = 2 + 2 * switch_count;
     expected_signature = Instruction::kSparseSwitchSignature;
   }
@@ -1329,19 +1329,33 @@ bool MethodVerifier::CheckSwitchTargets(uint32_t cur_offset) {
                                       << ", count " << insn_count;
     return false;
   }
-  /* for a sparse switch, verify the keys are in ascending order */
-  if (keys_offset > 0 && switch_count > 1) {
-    int32_t last_key = switch_insns[keys_offset] | (switch_insns[keys_offset + 1] << 16);
-    for (uint32_t targ = 1; targ < switch_count; targ++) {
-      int32_t key =
-          static_cast<int32_t>(switch_insns[keys_offset + targ * 2]) |
-          static_cast<int32_t>(switch_insns[keys_offset + targ * 2 + 1] << 16);
-      if (key <= last_key) {
-        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "invalid sparse switch: last key=" << last_key
-                                          << ", this=" << key;
+
+  constexpr int32_t keys_offset = 2;
+  if (switch_count > 1) {
+    if (is_packed_switch) {
+      /* for a packed switch, verify that keys do not overflow int32 */
+      int32_t first_key = switch_insns[keys_offset] | (switch_insns[keys_offset + 1] << 16);
+      int32_t max_first_key =
+          std::numeric_limits<int32_t>::max() - (static_cast<int32_t>(switch_count) - 1);
+      if (first_key > max_first_key) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "invalid packed switch: first_key=" << first_key
+                                          << ", switch_count=" << switch_count;
         return false;
       }
-      last_key = key;
+    } else {
+      /* for a sparse switch, verify the keys are in ascending order */
+      int32_t last_key = switch_insns[keys_offset] | (switch_insns[keys_offset + 1] << 16);
+      for (uint32_t targ = 1; targ < switch_count; targ++) {
+        int32_t key =
+            static_cast<int32_t>(switch_insns[keys_offset + targ * 2]) |
+            static_cast<int32_t>(switch_insns[keys_offset + targ * 2 + 1] << 16);
+        if (key <= last_key) {
+          Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "invalid sparse switch: last key=" << last_key
+                                            << ", this=" << key;
+          return false;
+        }
+        last_key = key;
+      }
     }
   }
   /* verify each switch target */

diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index 6568eac..17c1f00 100644 (file)
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -1,4 +1,6 @@
 PackedSwitch
+PackedSwitch key INT_MAX
+PackedSwitch key overflow
 b/17790197
 FloatBadArgReg
 negLong

diff --git a/test/800-smali/smali/PackedSwitch.smali b/test/800-smali/smali/PackedSwitch.smali
index 6a3e5f0..95659fb 100644 (file)
--- a/test/800-smali/smali/PackedSwitch.smali
+++ b/test/800-smali/smali/PackedSwitch.smali
@@ -24,3 +24,29 @@
     goto :return
 
 .end method
+
+.method public static packedSwitch_INT_MAX(I)I
+    .registers 2
+
+    const/4 v0, 0
+    packed-switch v0, :switch_data
+    goto :default
+
+    :switch_data
+    .packed-switch 0x7FFFFFFE
+        :case1  # key = INT_MAX - 1
+        :case2  # key = INT_MAX
+    .end packed-switch
+
+    :return
+    return v1
+
+    :default
+    goto :return
+
+    :case1
+    goto :return
+    :case2
+    goto :return
+
+.end method

diff --git a/test/800-smali/smali/b_24399945.smali b/test/800-smali/smali/b_24399945.smali
new file mode 100644 (file)
index 0000000..68f59d0
--- /dev/null
+++ b/test/800-smali/smali/b_24399945.smali
@@ -0,0 +1,32 @@
+.class public Lb_24399945;
+
+.super Ljava/lang/Object;
+
+.method public static packedSwitch_overflow(I)I
+    .registers 2
+
+    const/4 v0, 0
+    packed-switch v0, :switch_data
+    goto :default
+
+    :switch_data
+    .packed-switch 0x7FFFFFFE
+        :case1 # key = INT_MAX - 1
+        :case2 # key = INT_MAX
+        :case3 # key = INT_MIN (overflow!)
+    .end packed-switch
+
+    :return
+    return v1
+
+    :default
+    goto :return
+
+    :case1
+    goto :return
+    :case2
+    goto :return
+    :case3
+    goto :return
+
+.end method

diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index ba4990a..f75747d 100644 (file)
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -51,6 +51,10 @@ public class Main {
         testCases = new LinkedList<TestCase>();
         testCases.add(new TestCase("PackedSwitch", "PackedSwitch", "packedSwitch",
                 new Object[]{123}, null, 123));
+        testCases.add(new TestCase("PackedSwitch key INT_MAX", "PackedSwitch",
+                "packedSwitch_INT_MAX", new Object[]{123}, null, 123));
+        testCases.add(new TestCase("PackedSwitch key overflow", "b_24399945",
+                "packedSwitch_overflow", new Object[]{123}, new VerifyError(), null));
 
         testCases.add(new TestCase("b/17790197", "B17790197", "getInt", null, null, 100));
         testCases.add(new TestCase("FloatBadArgReg", "FloatBadArgReg", "getInt",