[lldb] Observe SG_READ_ONLY flag in MachO binaries

[kastiglione]

Summary

Change Section::GetPermissions() to indicate non-writability when the Mach-O
segment has the SG_READ_ONLY flag.

MachO has a segment flag named SG_READ_ONLY which indicates that the segment
is semantically read-only, even if its permissions are marked writable.
Segments such as __DATA_CONST contain data that is semantically "const", but
needs to be writable for dyld to perform fixups (bindings or rebases).

The header documentation for SG_READ_ONLY states:

This segment is made read-only after fixups.

[augusto2112]

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

1436

Could we add a new value in the enumeration? Something like ePermissionsLinkerWritable? As it is right now this would be dangerous for the existing file-cache optimization as we'd happily read pointers that are supposed to be fixed by the linker from the file-cache.

[kastiglione]

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

1436

That works for me. I think we'd want ePermissionsLoaderWritable.

[aprantl]

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

1436

That sounds good.

[kastiglione]

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

1436

Some idle questions:

1. Some code turns permissions into a string, like rw- or r-x, but there's no string character for this "hybrid" writable flag.
2. Other code might make r/w/x assumptions, and have bugs because of this extra state?
3. Do any other binary file formats have this notion, or would this be forcing a Mach-O specific flag into a portable concept?

I haven't looked yet, but I'm thinking it would be good if there were some other set of flags this information could be stored in.

I'm also not even sure if SG_READ_ONLY should modify permissions. For such a segment, should the permissions say it's writable, and something else say "actually no it's not", or should the permissions say it's non-writable, and something else says "actually it is written to by the loader".

[labath]

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

1436

Do any other binary file formats have this notion, or would this be forcing a Mach-O specific flag into a portable concept?

There is a PT_GNU_RELRO in linux land:

PT_GNU_RELRO	 	

The array element specifies the location and size of a segment which may be made read-only after relocations have been processed.

I haven't checked windows, but I'd expect it to have something similar -- security hardening is a hot topic these days and writable vtables are a tempting target.

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

 //===-- ObjectFileMachO.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
 //
 //===----------------------------------------------------------------------===//
 
void ObjectFileMachO::SanitizeSegmentCommand(
   }
 }
 
 static uint32_t
 GetSegmentPermissions(const llvm::MachO::segment_command_64 &seg_cmd) {
   uint32_t result = 0;
   if (seg_cmd.initprot & VM_PROT_READ)
     result |= ePermissionsReadable;
-  if (seg_cmd.initprot & VM_PROT_WRITE)
+  if ((seg_cmd.initprot & VM_PROT_WRITE) && !(seg_cmd.flags & SG_READ_ONLY))
     result |= ePermissionsWritable;
   if (seg_cmd.initprot & VM_PROT_EXECUTE)
     result |= ePermissionsExecutable;
   return result;
 }
 
 static lldb::SectionType GetSectionType(uint32_t flags,
                                         ConstString section_name) {

llvm/include/llvm/BinaryFormat/MachO.h

 //===-- llvm/BinaryFormat/MachO.h - The MachO file format -------*- C++/-*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //

 #undef HANDLE_LOAD_COMMAND
 
 enum : uint32_t {
   // Constant bits for the "flags" field in llvm::MachO::segment_command
   SG_HIGHVM = 0x1u,
   SG_FVMLIB = 0x2u,
   SG_NORELOC = 0x4u,
   SG_PROTECTED_VERSION_1 = 0x8u,
+  SG_READ_ONLY = 0x10u,
 
   // Constant masks for the "flags" field in llvm::MachO::section and
   // llvm::MachO::section_64
   SECTION_TYPE = 0x000000ffu,           // SECTION_TYPE
   SECTION_ATTRIBUTES = 0xffffff00u,     // SECTION_ATTRIBUTES
   SECTION_ATTRIBUTES_USR = 0xff000000u, // SECTION_ATTRIBUTES_USR
   SECTION_ATTRIBUTES_SYS = 0x00ffff00u  // SECTION_ATTRIBUTES_SYS
 };