llvm-objcopy calculates section locations incorrectly for MachO files

[peledins-zimperium]

https://github.com/llvm/llvm-project/pull/130517/files introduced a change

          if (RequiresFirstSectionOutsideFirstPage) {
            SectOffset = alignToPowerOf2(SectOffset, PageSize);
            RequiresFirstSectionOutsideFirstPage = false;
          }

This is an issue for several reasons:

1. If the first section is outside of first page (0x4000), but starts at (0x4004), then this gets aligned to (0x8000), which is not necessary. Here a fix could be

if (RequiresFirstSectionOutsideFirstPage && SectOffset < PageSize) {

2. However, if the block is actually entered, then trouble happens with next sections: they do not get moved together with this section. Then we can get to a state of truncated or malformed object (section contents at offset 20160 with a size of 111316, overlaps section contents at offset 32768 with a size of 228)

[peledins-zimperium]

Repro.
repro.yaml (a binary where unwind section starts at 0x8000, but __text at 0x4004)

--- !mach-o
FileHeader:
  magic:           0xFEEDFACF
  cputype:         0x100000C
  cpusubtype:      0x0
  filetype:        0x2
  ncmds:           15
  sizeofcmds:      752
  flags:           0x200085
  reserved:        0x0
LoadCommands:
  - cmd:             LC_SEGMENT_64
    cmdsize:         72
    segname:         __PAGEZERO
    vmaddr:          0
    vmsize:          4294967296
    fileoff:         0
    filesize:        0
    maxprot:         0
    initprot:        0
    nsects:          0
    flags:           0
  - cmd:             LC_SEGMENT_64
    cmdsize:         232
    segname:         __TEXT
    vmaddr:          4294967296
    vmsize:          49152
    fileoff:         0
    filesize:        49152
    maxprot:         5
    initprot:        5
    nsects:          2
    flags:           0
    Sections:
      - sectname:        __text
        segname:         __TEXT
        addr:            0x100004004
        size:            16380
        offset:          0x4004
        align:           2
        reloff:          0x0
        nreloc:          0
        flags:           0x80000400
        reserved1:       0x0
        reserved2:       0x0
        reserved3:       0x0
      - sectname:        __unwind_info
        segname:         __TEXT
        addr:            0x100008000
        size:            96
        offset:          0x8000
        align:           2
        reloff:          0x0
        nreloc:          0
        flags:           0x0
        reserved1:       0x0
        reserved2:       0x0
        reserved3:       0x0
  - cmd:             LC_SEGMENT_64
    cmdsize:         72
    segname:         __LINKEDIT
    vmaddr:          4295016448
    vmsize:          16384
    fileoff:         49152
    filesize:        176
    maxprot:         1
    initprot:        1
    nsects:          0
    flags:           0
  - cmd:             LC_DYLD_INFO_ONLY
    cmdsize:         48
    rebase_off:      0
    rebase_size:     0
    bind_off:        0
    bind_size:       0
    weak_bind_off:   0
    weak_bind_size:  0
    lazy_bind_off:   0
    lazy_bind_size:  0
    export_off:      49152
    export_size:     56
  - cmd:             LC_SYMTAB
    cmdsize:         24
    symoff:          49216
    nsyms:           4
    stroff:          49280
    strsize:         48
  - cmd:             LC_DYSYMTAB
    cmdsize:         80
    ilocalsym:       0
    nlocalsym:       0
    iextdefsym:      0
    nextdefsym:      3
    iundefsym:       3
    nundefsym:       1
    tocoff:          0
    ntoc:            0
    modtaboff:       0
    nmodtab:         0
    extrefsymoff:    0
    nextrefsyms:     0
    indirectsymoff:  0
    nindirectsyms:   0
    extreloff:       0
    nextrel:         0
    locreloff:       0
    nlocrel:         0
  - cmd:             LC_LOAD_DYLINKER
    cmdsize:         32
    name:            12
    Content:         '/usr/lib/dyld'
    ZeroPadBytes:    7
  - cmd:             LC_UUID
    cmdsize:         24
    uuid:            18DC1424-F10E-335F-B440-2A4B968C874D
  - cmd:             LC_VERSION_MIN_IPHONEOS
    cmdsize:         16
    version:         458752
    sdk:             1180672
  - cmd:             LC_SOURCE_VERSION
    cmdsize:         16
    version:         0
  - cmd:             LC_MAIN
    cmdsize:         24
    entryoff:        16384
    stacksize:       0
  - cmd:             LC_ENCRYPTION_INFO_64
    cmdsize:         24
    cryptoff:        16384
    cryptsize:       32768
    cryptid:         0
    pad:             0
  - cmd:             LC_LOAD_DYLIB
    cmdsize:         56
    dylib:
      name:            24
      timestamp:       2
      current_version: 88539136
      compatibility_version: 65536
    Content:         '/usr/lib/libSystem.B.dylib'
    ZeroPadBytes:    6
  - cmd:             LC_FUNCTION_STARTS
    cmdsize:         16
    dataoff:         49208
    datasize:        8
  - cmd:             LC_DATA_IN_CODE
    cmdsize:         16
    dataoff:         49216
    datasize:        0
LinkEditData:
  ExportTrie:
    TerminalSize:    0
    NodeOffset:      0
    Name:            ''
    Flags:           0x0
    Address:         0x0
    Other:           0x0
    ImportName:      ''
    Children:
      - TerminalSize:    0
        NodeOffset:      25
        Name:            _
        Flags:           0x0
        Address:         0x0
        Other:           0x0
        ImportName:      ''
        Children:
          - TerminalSize:    2
            NodeOffset:      9
            Name:            _mh_execute_header
            Flags:           0x0
            Address:         0x0
            Other:           0x0
            ImportName:      ''
          - TerminalSize:    4
            NodeOffset:      13
            Name:            main
            Flags:           0x0
            Address:         0x4000
            Other:           0x0
            ImportName:      ''
          - TerminalSize:    4
            NodeOffset:      19
            Name:            x
            Flags:           0x0
            Address:         0x4008
            Other:           0x0
            ImportName:      ''
  NameList:
    - n_strx:          2
      n_type:          0xF
      n_sect:          1
      n_desc:          16
      n_value:         4294967296
    - n_strx:          22
      n_type:          0xF
      n_sect:          1
      n_desc:          0
      n_value:         4294983680
    - n_strx:          28
      n_type:          0xF
      n_sect:          1
      n_desc:          0
      n_value:         4294983688
    - n_strx:          31
      n_type:          0x1
      n_sect:          0
      n_desc:          256
      n_value:         0
  StringTable:
    - ' '
    - __mh_execute_header
    - _main
    - _x
    - dyld_stub_binder
  FunctionStarts:  [ 0x4000, 0x4008 ]
...

Run:

yaml2obj repro.yaml -o repro.out
llvm-strip --strip-all ./repro.out -o repro.stripped.out
llvm-otool -l ./repro.stripped.out
llvm-otool: error: './repro.stripped.out': truncated or malformed object (section contents at offset 32768 with a size of 96, overlaps section contents at offset 32768 with a size of 16380)

[llvmbot]

@llvm/issue-subscribers-tools-llvm-objcopy-strip

Author: None (peledins-zimperium)

https://github.com//pull/130517/files introduced a change

          if (RequiresFirstSectionOutsideFirstPage) {
            SectOffset = alignToPowerOf2(SectOffset, PageSize);
            RequiresFirstSectionOutsideFirstPage = false;
          }

This is an issue for several reasons:

1. If the first section is outside of first page (0x4000), but starts at (0x4004), then this gets aligned to (0x8000), which is not necessary. Here a fix could be

if (RequiresFirstSectionOutsideFirstPage && SectOffset < PageSize) {

2. However, if the block is actually entered, then trouble happens with next sections: they do not get moved together with this section. Then we can get to a state of truncated or malformed object (section contents at offset 20160 with a size of 111316, overlaps section contents at offset 32768 with a size of 228)

[peledins-zimperium]

It is unclear to me in which cases the page alignment/shift should be done, but in general case of good binary being stripped we shouldn't do it: for example - it can be a case that the code refers to the macho header by relative address - if the shift happens, then that would need to be adjusted.

The partial proposed fix (and current alignment as well) are not correct in another way as well. What if the load commands take more than one page?

[drodriguez]

What about changing it to:

// Around line 118
uint64_t Offset = IsObjectFile ? (HeaderSize + O.Header.SizeOfCmds) : 0;
// If we are emitting an encryptable binary, our load commands must have a
// separate (non-encrypted) page to themselves.
bool RequiresFirstSectionOutsideFirstPage = O.EncryptionInfoCommandIndex.has_value();
uint64_t FirstSectionMinimumOffset = RequiresFirstSectionOutsideFirstPage ? alignToPowerOf2(Offset, PageSize) : 0;

// Around line 176
if (RequiresFirstSectionOutsideFirstPage) {
    SectOffset = SectOffset < FirstSectionMinimumOffset ? FirstSectionMinimumOffset : SectOffset;
    RequiresFirstSectionOutsideFirstPage = false;
}

I have not run this, but it should:

• handle when there's no encryptable sections requirement
• handle the case in which SizeOfCmds is below the page size
• handle the case in which the first section offset is already beyond the page size
• handle the case in which the commands are larger than 1 page and we want to move encryptable sections to the second page boundary.

[peledins-zimperium]

I am not sure this works well:

1. What happens to sections after the first one? (I think the offsets are not accounted for correctly).
2. RequiresFirstSectionOutsideFirstPage could probably be named RequiresFirstSectionOutsideOfHeaderPages?

I see the failure in llvm-strip: in such case it would be safe to assume that the existing binary is well-formed, so this alignment to sections would not need to happen at all.

What is the scenario you are concerned with? Is it a case when there is more than one section?