[Support] Fix undefined shift in decodeULEB128/decodeSLEB128 for overlong encodings

[qiyao]

When a (signed or unsigned) LEB128 value is encoded with extra trailing
bytes that only carry zero- or sign-extension, the decode loop could keep
running with the shift amount at 64 or beyond and then evaluate
Slice << Shift, which is undefined behavior for a 64-bit type.

The DWARF expression parser feeds attacker-controlled LEB128 operands
(such as DW_OP_bregN / DW_OP_constu) through
DataExtractor::getULEB128 / getSLEB128, so the
lldb-dwarf-expression-fuzzer reaches this under UBSan. The unsigned
case:

LEB128.h:152:20: runtime error: shift exponent 70 is too large for
64-bit type 'uint64_t' (aka 'unsigned long long')
    #0 llvm::decodeULEB128(...) LEB128.h:152
    #1 getLEB128<unsigned long long>(...) DataExtractor.cpp:227
    #2 llvm::DataExtractor::getULEB128(...) DataExtractor.cpp:241
    #3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:218
    #7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333
    #9 LLVMFuzzerTestOneInput lldb-dwarf-expression-fuzzer.cpp:83
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior LEB128.h:152:20

and the signed case, reached the same way:

LEB128.h:190:20: runtime error: shift exponent 70 is too large for
64-bit type 'uint64_t' (aka 'unsigned long long')
    #0 llvm::decodeSLEB128(...) LEB128.h:190
    #1 getLEB128<long long>(...) DataExtractor.cpp:227
    #2 llvm::DataExtractor::getSLEB128(...) DataExtractor.cpp:245
    #3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:216
    #7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333

The existing range checks already guarantee that once Shift reaches 64
the remaining bytes are pure extension and contribute nothing to the
result, so skip the accumulating shift in that case. Decoded values are
unchanged for all well-formed inputs.

Adds overlong-encoding regression cases to LEB128Test. Without the fix
the signed case is the UBSan diagnostic above in a sanitizer build, and
in a normal build the unsigned case also decodes to the wrong value (the
overlong 1 decodes as 11).

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-support

Author: Yao Qi (qiyao)

Changes

When a (signed or unsigned) LEB128 value is encoded with extra trailing
bytes that only carry zero- or sign-extension, the decode loop could keep
running with the shift amount at 64 or beyond and then evaluate
Slice << Shift, which is undefined behavior for a 64-bit type.

The DWARF expression parser feeds attacker-controlled LEB128 operands
(such as DW_OP_bregN / DW_OP_constu) through
DataExtractor::getULEB128 / getSLEB128, so the
lldb-dwarf-expression-fuzzer reaches this under UBSan. The unsigned
case:

LEB128.h:152:20: runtime error: shift exponent 70 is too large for
64-bit type 'uint64_t' (aka 'unsigned long long')
    #<!-- -->0 llvm::decodeULEB128(...) LEB128.h:152
    #<!-- -->1 getLEB128&lt;unsigned long long&gt;(...) DataExtractor.cpp:227
    #<!-- -->2 llvm::DataExtractor::getULEB128(...) DataExtractor.cpp:241
    #<!-- -->3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:218
    #<!-- -->7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333
    #<!-- -->9 LLVMFuzzerTestOneInput lldb-dwarf-expression-fuzzer.cpp:83
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior LEB128.h:152:20

and the signed case, reached the same way:

LEB128.h:190:20: runtime error: shift exponent 70 is too large for
64-bit type 'uint64_t' (aka 'unsigned long long')
    #<!-- -->0 llvm::decodeSLEB128(...) LEB128.h:190
    #<!-- -->1 getLEB128&lt;long long&gt;(...) DataExtractor.cpp:227
    #<!-- -->2 llvm::DataExtractor::getSLEB128(...) DataExtractor.cpp:245
    #<!-- -->3 llvm::DWARFExpression::Operation::extract(...) DWARFExpression.cpp:216
    #<!-- -->7 lldb_private::DWARFExpression::Evaluate(...) DWARFExpression.cpp:1333

The existing range checks already guarantee that once Shift reaches 64
the remaining bytes are pure extension and contribute nothing to the
result, so skip the accumulating shift in that case. Decoded values are
unchanged for all well-formed inputs.

Adds overlong-encoding regression cases to LEB128Test. Without the fix
the signed case is the UBSan diagnostic above in a sanitizer build, and
in a normal build the unsigned case also decodes to the wrong value (the
overlong 1 decodes as 11).

---

Full diff: https://github.com/llvm/llvm-project/pull/205907.diff

2 Files Affected:

• (modified) llvm/include/llvm/Support/LEB128.h (+10-2)
• (modified) llvm/unittests/Support/LEB128Test.cpp (+16)

diff --git a/llvm/include/llvm/Support/LEB128.h b/llvm/include/llvm/Support/LEB128.h
index 0b8cb6781aec4..f11abafafa4b7 100644
--- a/llvm/include/llvm/Support/LEB128.h
+++ b/llvm/include/llvm/Support/LEB128.h
@@ -149,7 +149,11 @@ inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
       Value = 0;
       break;
     }
-    Value += Slice << Shift;
+    // Once Shift reaches 64 the remaining bytes have already been validated
+    // above to be pure zero-extension, so they contribute nothing. Performing
+    // "Slice << Shift" with Shift >= 64 would be undefined behavior, so skip it.
+    if (LLVM_LIKELY(Shift < 64))
+      Value += Slice << Shift;
     Shift += 7;
   } while (*p++ >= 128);
   if (n)
@@ -187,7 +191,11 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
         *n = (unsigned)(p - orig_p);
       return 0;
     }
-    Value |= Slice << Shift;
+    // Once Shift reaches 64 the remaining bytes have already been validated
+    // above to be pure sign-extension, so they contribute nothing. Performing
+    // "Slice << Shift" with Shift >= 64 would be undefined behavior, so skip it.
+    if (LLVM_LIKELY(Shift < 64))
+      Value |= Slice << Shift;
     Shift += 7;
     ++p;
   } while (Byte >= 128);
diff --git a/llvm/unittests/Support/LEB128Test.cpp b/llvm/unittests/Support/LEB128Test.cpp
index 0c54a2846903b..db6cf0a38edaa 100644
--- a/llvm/unittests/Support/LEB128Test.cpp
+++ b/llvm/unittests/Support/LEB128Test.cpp
@@ -139,6 +139,13 @@ TEST(LEB128Test, DecodeULEB128) {
   EXPECT_DECODE_ULEB128_EQ(0x80000000'00000000ul,
                            "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x01");
 
+  // Decode overlong ULEB128 whose trailing zero-extension bytes push the shift
+  // amount to 64 or beyond.
+  EXPECT_DECODE_ULEB128_EQ(
+      0u, "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x00");
+  EXPECT_DECODE_ULEB128_EQ(
+      1u, "\x81\x80\x80\x80\x80\x80\x80\x80\x80\x80\x00");
+
 #undef EXPECT_DECODE_ULEB128_EQ
 }
 
@@ -216,6 +223,15 @@ TEST(LEB128Test, DecodeSLEB128) {
   EXPECT_DECODE_SLEB128_EQ(INT64_MAX,
                            "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00");
 
+  // Decode overlong SLEB128 whose trailing sign-extension bytes push the shift
+  // amount to 64 or beyond.
+  EXPECT_DECODE_SLEB128_EQ(
+      0L, "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x00");
+  EXPECT_DECODE_SLEB128_EQ(
+      -1L, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x7F");
+  EXPECT_DECODE_SLEB128_EQ(
+      -2L, "\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x7F");
+
 #undef EXPECT_DECODE_SLEB128_EQ
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🐧 Linux x64 Test Results

• 197395 tests passed
• 5446 tests skipped

✅ The build succeeded and all tests passed.

[qiyao]

Ping authors and reviewers on the LEB128 decode error-handling code most recently, @MaskRay, @adrian-prantl, @dwblaikie. Please review it.

[dwblaikie]

Looks about right to me - thanks!

[qiyao]

@dwblaikie , thanks for the approval. I requested commit access, but haven't got it yet. Can you help to merge it?