[LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

[cmtice]

Summary

In two calls to ReadMemory in DWARFExpression.cpp, the buffer size passed to ReadMemory is not checked and can be bigger than the actual size of the buffer. This caused a buffer overflow bug, which we found through Address Sanitizer. This patch fixes the problem by checking the address size when it is first read out of the DWARF, and setting an error and returning immediately if the size is invalid.

This is the second attempt to fix this issue; I reverted the first one, as it was not quite correct.

[jasonmolenda]

This looks good to me, thanks for digging in Caroline! Is there a naughty compiler emitting this, or are we mis-parsing somehow?

[cmtice]

Thanks Jason. I think the compiler is generating some bad DWARF. David Blaikie was investigating that on our end, but he's on vacation this week.

[jasonmolenda]

Thanks!

[dblaikie]

In D154907#4487335, @jasonmolenda wrote:

This looks good to me, thanks for digging in Caroline! Is there a naughty compiler emitting this, or are we mis-parsing somehow?

In D154907#4487523, @cmtice wrote:

Thanks Jason. I think the compiler is generating some bad DWARF. David Blaikie was investigating that on our end, but he's on vacation this week.

Yep - something in libc++ built with clang:

              DW_AT_name        ("third_party/llvm/llvm-project/libcxx/src/ios.instantiations.cpp")
                     [0x000000002b32cf96, 0x000000002b32d0e9): DW_OP_breg6 RBP-52, DW_OP_deref_size 0x10, DW_OP_stack_value)
                     [0x000000002b32d116, 0x000000002b32d265): DW_OP_breg6 RBP-56, DW_OP_deref_size 0x10, DW_OP_stack_value)
                        DW_AT_location  (DW_OP_fbreg -56, DW_OP_deref_size 0x10, DW_OP_stack_value)
                     [0x000000002b331396, 0x000000002b3314e6): DW_OP_breg6 RBP-52, DW_OP_deref_size 0x10, DW_OP_stack_value)
                     [0x000000002b331516, 0x000000002b331662): DW_OP_breg6 RBP-56, DW_OP_deref_size 0x10, DW_OP_stack_value)
                        DW_AT_location  (DW_OP_fbreg -56, DW_OP_deref_size 0x10, DW_OP_stack_value)
--
              DW_AT_name        ("third_party/llvm/llvm-project/libcxx/src/locale.cpp")
                     [0x000000002b34146b, 0x000000002b341623): DW_OP_breg6 RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
                     [0x000000002b3416d7, 0x000000002b3417e7): DW_OP_breg6 RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
                     [0x000000002b342b2b, 0x000000002b342cdd): DW_OP_breg6 RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value
                     [0x000000002b342d96, 0x000000002b342ea6): DW_OP_breg6 RBP-48, DW_OP_deref_size 0x10, DW_OP_stack_value

I haven't isolated this - got to extract/figure out how our libc++ is built, etc.

lldb/source/Expression/DWARFExpression.cpp

1082–1089

Just as an aside - isn't this code doing an illegal load widening? If the pointer pointed to the end of a page or something, and asked for only one byte - reading extra bytes would be bad (similarly would cause a segfault/UB/etc), right?

(& I'm not sure I understand the comment about endianness - the operation reads that many bytes from the given address)

[dblaikie]

lldb/source/Expression/DWARFExpression.cpp

1082–1089

oh, guess I also mentioned this here: https://reviews.llvm.org/D153840#inline-1494202

[dblaikie]

Simple clang example that produces this invalid DWARF:

void b(double);
void c();
void e(double e) {
  c();
  b(e);
}

$ clang-tot x.ii -g -c -o - -O3 | llvm-dwarfdump-tot - | grep DW_OP_deref_size
                     [0x0000000000000006, 0x0000000000000016): DW_OP_breg7 RSP+0, DW_OP_deref_size 0x10, DW_OP_stack_value)

This seems to be created here: https://github.com/llvm/llvm-project/blob/dd84f5f91c6b234a2f188b6acf8557cae81b8a53/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp#L1281 - so I'll file a bug for @jmorse to take a look at, perhaps.

[dblaikie]

Filed https://github.com/llvm/llvm-project/issues/64093

lldb/source/Expression/DWARFExpression.cpp

while (opcodes.ValidOffset(offset)) {
     case DW_OP_deref_size: {
       if (stack.empty()) {
         if (error_ptr)
           error_ptr->SetErrorString(
               "Expression stack empty for DW_OP_deref_size.");
         return false;
       }
       uint8_t size = opcodes.GetU8(&offset);
+      if (size > 8) {
+        if (error_ptr)
+              error_ptr->SetErrorStringWithFormat(
+                  "Invalid address size for DW_OP_deref_size: %d\n",
+                  size);
+        return false;
+      }
       Value::ValueType value_type = stack.back().GetValueType();
       switch (value_type) {
       case Value::ValueType::HostAddress: {
         void *src = (void *)stack.back().GetScalar().ULongLong();
         intptr_t ptr;
         ::memcpy(&ptr, src, sizeof(void *));
         // I can't decide whether the size operand should apply to the bytes in
         // their
         // lldb-host endianness or the target endianness.. I doubt this'll ever
         // come up but I'll opt for assuming big endian regardless.
         switch (size) {
         case 1:
           ptr = ptr & 0xff;
           break;
         case 2:
           ptr = ptr & 0xffff;
           break;
         case 3:
           ptr = ptr & 0xffffff;
case DW_OP_deref_size: {
 
             stack.back().GetScalar() = DerefSizeExtractDataHelper(
                 addr_bytes, size, objfile->GetByteOrder(), size);
             stack.back().ClearContext();
             break;
           } else {
             if (error_ptr)
               error_ptr->SetErrorStringWithFormat(
-                  "Failed to dereference pointer for for DW_OP_deref_size: "
+                  "Failed to dereference pointer for DW_OP_deref_size: "
                   "%s\n",
                   error.AsCString());
             return false;
           }
         }
         stack.back().GetScalar() = load_addr;
         // Fall through to load address promotion code below.
       }