[PATCH] D139184: [LLD][Windows]Feature "checksum" for Windows PE

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D139184#4016073 <https://reviews.llvm.org/D139184#4016073>, @tonic wrote:

> In D139184#4016012 <https://reviews.llvm.org/D139184#4016012>, @Qfrost911 wrote:
>
>> I deem that it is impossible to leak source code because the "checksum" only occupies only a DWORD. If reviewers think there is no problem, please accept the patch so that I can submit this commit.
>
> This about the implementation of checksum. You mentioned that you had looked at leaked nt5 source code. As long as no code has been taken or copied from copyrighted nt5 source, then it is ok from a legal perspective to be included.

If someone was to take a look at that code, they would see it is different from what it is currently written in this patch.

The algorithm used for the PE checksum is essentially RFC1071 <http://www.faqs.org/rfcs/rfc1071.html>, as documented here <https://www.codeproject.com/Articles/19326/An-Analysis-of-the-Windows-PE-Checksum-Algorithm> and here <https://practicalsecurityanalytics.com/pe-checksum/>. We already have an in implementation in libc, here <https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/libc/AOR_v20.02/networking/test/chksum.c#L40>, but I wouldn't touch or move it.

I think the whole algorithm can be implemented more simply just as it is described in RFC1071:

  void Writer::writePEChecksum() {
    if (!config->writeCheckSum) {
      return;
    }
  
    // https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#checksum
    uint32_t *buf = (uint32_t *)buffer->getBufferStart();
    uint32_t size = (uint32_t)(buffer->getBufferSize());
  
    coff_file_header *coffHeader =
        (coff_file_header *)((uint8_t *)buf + dosStubSize + sizeof(PEMagic));
    pe32_header *peHeader =
        (pe32_header *)((uint8_t *)coffHeader + sizeof(coff_file_header));
  
    uint64_t sum = 0;
    uint32_t count = size;
    uint16_t *addr = (uint16_t *)buf;
  
    while (count > 1) {
      sum += *addr++;
      count -= 2;
    }
  
    // Add left-over byte, if any
    if (count > 0)
      sum += *(unsigned char *)addr;
  
    // Fold 32-bit sum to 16 bits
    while (sum >> 16)
      sum = (sum & 0xffff) + (sum >> 16);
  
    sum += size;
    peHeader->CheckSum = sum;
  }

And probably a comment that mentions a link to the RFC.

I've tested with a simple program:

  C:\git\llvm-project>cat main.cpp
  int main() { return 0; }
  
  C:\git\llvm-project>stage1\bin\clang-cl main.cpp /link /RELEASE /Brepro
  
  C:\git\llvm-project>stage1\bin\llvm-readobj.exe --all main.exe | grep CheckSum
    CheckSum: 0x1D26D

(which is the same value as the implementation in the current patch)



================
Comment at: lld/COFF/Writer.cpp:617
+      (pe32_header *)((uint8_t *)coffHeader + sizeof(coff_file_header));
+  uint32_t oldCheckSum = peHeader->CheckSum;
+
----------------
mstorsjo wrote:
> What is `oldChecksum` initialized to here, practically?
@Qfrost911 I wouldn't worry about the old checksum, in practical terms is should be always 0. Its maybe just the kernel that needs it for hotpatching perhaps? In our case it doesn't matter I think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139184/new/

https://reviews.llvm.org/D139184