When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

[jasonmolenda]

Summary

The Darwin kernel has a number of kernel extensions, kexts, akin to solibs to lldb. We find the kext binary on disk, we create a MemoryModule from the in-memory image based on the Mach-O header and Mach-O load commands in memory. The LC_SEGMENT vmaddrs for each segment in memory have been updated with their actual load addresses. DynamicLoaderDarwinKernel uses the MemoryModule's Section "file" addresses to set the load addresses for the on-disk Module in the target.

There are some extensions that can be loaded into memory which won't have the Mach-O load commands with their load addresses updated. So the MemoryModule has the original file addresses, instead of the actual in-memory load addresses. The current DynamicLoader code will then set the target section load addresses to the file addresses, which doesn't work.

This patch detects this by looking at the MemoryModule's Mach-O header (__TEXT) segment, and comparing its actual memory address we used to create the MemoryModule with the vmaddr ("file address") of the MemoryModule Section. If they differ, this is one of those binaries that hasn't had its load commands updated when they were added to memory.

When this is detected, we assume all segments slide by a constant value, calculate that slide, and apply it to all the sections. Normal kexts have their load addresses calculated as they always had been.

[jasonmolenda]

Show an alternate approach, where we check the file types of everything in the kernel+kext list, and if something is not a kernel/kext, don't try to load it at all. v. the first change in DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule where I call !IsKernel() && !obj_macho->IsKext() to detect something in the list which isn't processed the standard way that kexts & the kernel are. Either one would work in this case.

[jasonmolenda]

Add an additional bit to the "don't load non-kexts, non-kernels" addition. I refined this by checking to see if the binary is already registered in the Target, and already has a load address set. If the binary is, then we don't re-register it.

If we encounter a program that is a non-kext, non-kernel binary and isn't yet loaded in the Target, we will load it. So the code that handles in-memory load commands with incorrect vmaddrs is still needed.

[JDevlieghere]

Some code style nits but the change itself LGTM.

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

819–821

You can use dyn_cast_or_null to eliminate the null check:

ObjectFileMachO *ondisk_objfile_macho = llvm::dyn_cast_or_null<ObjectFileMachO>(ondisk_object_file);

I hate temporaries that are used only once below, so I'd probably write:

ObjectFileMachO *ondisk_objfile_macho = llvm::dyn_cast_or_null<ObjectFileMachO>(m_module_sp ? m_module_sp->GetObjectFile() : nullptr);

but I'm probably the only one that likes that better.

835

What's the purpose of reseting the shared pointer? It'll just go out of scope right after.

855–857

Since you already did it for Section below...

if(ObjectFileMachO *memory_objfile_macho =
            llvm::dyn_cast<ObjectFileMachO>(memory_object_file)) {

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

1146 ↗

(On Diff #503550)

Newline below

[jasonmolenda]

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

835

No, this method is trying a few different ways to find the on-disk binary for the kext, and it puts that Module in m_module_sp once it finds it. (there's a bunch of blocks that start like if (!m_module_sp) { ... try another way of finding the Module ...} and the goal when we have a non-kext/non-kernel image that is already in the Target is to avoid re-loading it into the Target. So by clearing it, we're behaving as if it couldn't be found.

altho, tbh, looking this method I could just return here and it would accomplish the same. And be clearer.

855–857

yeah, I thought about that but it made for a really long line that would be a multi-line expression in the if and i wasn't thrilled about it. but i don't care that much.

[jasonmolenda]

Update for Jonas' suggestions.

[bulbazord]

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

816–819

I have a suggestion that may make this easier to read. You already know that m_module_sp is valid because you have that as a condition to even enter this block. So you can do something like

ObjectFileMachO *ondisk_objfile_macho = m_module_sp->GetObjectFile();

But right below you only act on this if the object is valid, so you could stick it in the if condition itself:

if (auto *ondisk_objfile_macho = m_module_sp->GetObjectFile()) {

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt

add_lldb_library(lldbPluginDynamicLoaderDarwinKernel PLUGIN
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbHost
     lldbInterpreter
     lldbSymbol
     lldbTarget
     lldbUtility
+    lldbPluginObjectFileMachO
   )
 
 add_dependencies(lldbPluginDynamicLoaderDarwinKernel
   LLDBPluginDynamicLoaderDarwinKernelPropertiesGen
   LLDBPluginDynamicLoaderDarwinKernelPropertiesEnumGen)

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

 //===-- DynamicLoaderDarwinKernel.cpp -------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamFile.h"

 }
 
 void DynamicLoaderDarwinKernel::KextImageInfo::SetIsKernel(bool is_kernel) {
   m_kernel_image = is_kernel;
 }
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
     Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
     return true;
 
   Target &target = process->GetTarget();
 
   // kexts will have a uuid from the table.
   // for the kernel, we'll need to read the load commands out of memory to get it.
   if (m_uuid.IsValid() == false) {

       if (IsKernel() && !m_module_sp) {
         Stream &s = target.GetDebugger().GetOutputStream();
         s.Printf("WARNING: Unable to locate kernel binary on the debugger "
                  "system.\n");
       }
     }
 
     // If we managed to find a module, append it to the target's list of
     // images. If we also have a memory module, require that they have matching
     // UUIDs
     if (m_module_sp) {
       if (m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
         target.GetImages().AppendIfNeeded(m_module_sp, false);
       }
     }
   }
 
   // If we've found a binary, read the load commands out of memory so we
   // can set the segment load addresses.
   if (m_module_sp)
     ReadMemoryModule (process);
 
   static ConstString g_section_name_LINKEDIT("__LINKEDIT");
 
   if (m_memory_module_sp && m_module_sp) {
     if (m_module_sp->GetUUID() == m_memory_module_sp->GetUUID()) {
       ObjectFile *ondisk_object_file = m_module_sp->GetObjectFile();
       ObjectFile *memory_object_file = m_memory_module_sp->GetObjectFile();
 
       if (memory_object_file && ondisk_object_file) {
         // The memory_module for kexts may have an invalid __LINKEDIT seg; skip
         // it.
         const bool ignore_linkedit = !IsKernel();
 
+        // Normally a kext will have its segment load commands
+        // (LC_SEGMENT vmaddrs) corrected in memory to have their
+        // actual segment addresses.
+        // Userland proceses have their libraries updated the same way
+        // by dyld.  The Mach-O load commands in memory are the canonical
+        // addresses.
+        //
+        // If the kernel gives us a binary where the in-memory segment
+        // vmaddr is incorrect, then this binary was put in memory without
+        // updating its Mach-O load commands.  We should assume a static
+        // slide value will be applied to every segment; we don't have the
+        // correct addresses for each individual segment.
+        addr_t fixed_slide = LLDB_INVALID_ADDRESS;
+        ObjectFileMachO *memory_module_macho_sp =
+            llvm::dyn_cast<ObjectFileMachO>(memory_object_file);
+        if (memory_module_macho_sp) {
+          if (Section *header_sect =
+                  memory_module_macho_sp->GetMachHeaderSection()) {
+            if (header_sect->GetFileAddress() != m_load_address) {
+              fixed_slide = m_load_address - header_sect->GetFileAddress();
+              LLDB_LOGF(
+                  log,
+                  "kext %s in-memory LC_SEGMENT vmaddr is not correct, using a "
+                  "fixed slide of 0x%" PRIx64,
+                  m_name.c_str(), fixed_slide);
+            }
+          }
+        }
+
         SectionList *ondisk_section_list = ondisk_object_file->GetSectionList();
         SectionList *memory_section_list = memory_object_file->GetSectionList();
         if (memory_section_list && ondisk_section_list) {
           const uint32_t num_ondisk_sections = ondisk_section_list->GetSize();
           // There may be CTF sections in the memory image so we can't always
           // just compare the number of sections (which are actually segments
           // in mach-o parlance)
           uint32_t sect_idx = 0;

               // mapped into memory and there is no current way to tell.
               // I filed rdar://problem/12851706 to track being able to tell
               // if the __LINKEDIT is actually mapped, but until then, we need
               // to not load the __LINKEDIT
               if (ignore_linkedit &&
                   ondisk_section_sp->GetName() == g_section_name_LINKEDIT)
                 continue;
 
-              const Section *memory_section =
-                  memory_section_list
-                      ->FindSectionByName(ondisk_section_sp->GetName())
-                      .get();
-              if (memory_section) {
-                target.SetSectionLoadAddress(ondisk_section_sp,
-                                             memory_section->GetFileAddress());
-                ++num_sections_loaded;
+              if (fixed_slide != LLDB_INVALID_ADDRESS) {
+                target.SetSectionLoadAddress(
+                    ondisk_section_sp,
+                    ondisk_section_sp->GetFileAddress() + fixed_slide);
+              } else {
+                const Section *memory_section =
+                    memory_section_list
+                        ->FindSectionByName(ondisk_section_sp->GetName())
+                        .get();
+                if (memory_section) {
+                  target.SetSectionLoadAddress(
+                      ondisk_section_sp, memory_section->GetFileAddress());
+                  ++num_sections_loaded;
+                }
               }
             }
           }
           if (num_sections_loaded > 0)
             m_load_process_stop_id = process->GetStopID();
           else
             m_module_sp.reset(); // No sections were loaded
         } else

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

public:
   bool CanTrustAddressRanges() override;
 
   static bool ParseHeader(lldb_private::DataExtractor &data,
                           lldb::offset_t *data_offset_ptr,
                           llvm::MachO::mach_header &header);
 
   bool AllowAssemblyEmulationUnwindPlans() override;
 
+  lldb_private::Section *GetMachHeaderSection();
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
 protected:
   static lldb_private::UUID
   GetUUID(const llvm::MachO::mach_header &header,
           const lldb_private::DataExtractor &data,
           lldb::offset_t lc_offset); // Offset to the first load command
protected:
 
   /// Intended for same-host arm device debugging where lldb will read
   /// shared cache libraries out of its own memory instead of the remote
   /// process' memory as an optimization.  If lldb's shared cache UUID
   /// does not match the process' shared cache UUID, this optimization
   /// should not be used.
   void GetLLDBSharedCacheUUID(lldb::addr_t &base_addir, lldb_private::UUID &uuid);
 
-  lldb_private::Section *GetMachHeaderSection();
-
   lldb::addr_t CalculateSectionLoadAddressForMemoryImage(
       lldb::addr_t mach_header_load_address,
       const lldb_private::Section *mach_header_section,
       const lldb_private::Section *section);
 
   lldb_private::UUID
   GetSharedCacheUUID(lldb_private::FileSpec dyld_shared_cache,
                      const lldb::ByteOrder byte_order,