[LLD][Windows]Feature "checksum" for Windows PE

[Qfrost911]

Summary

There is a field named checksum in PE, in order to check file whether be corrupted. Sometimes, VisualStudio will defaultly send this option, RELEASE, to lld. So, a error will be raised

lld-link : error : could not open '/RELEASE': no such file or directory

The general idea is to generate the binary and then recalculate the checksum before outputting it.

[aaron.ballman]

I don't know enough about lld to be an appropriate reviewer for this, but the PE bits look correct based on my (limited) understanding of PE.

lld/COFF/Writer.cpp

621

Something to double-check: is BaseAddress going to be aligned properly for dereferencing p? I believe this is fine, but it's worth verifying.

[mstorsjo]

lld/COFF/Writer.cpp

617

What is oldChecksum initialized to here, practically?

633

Is size + 1 here meant for rounding up? Shouldn’t it then be sizeof(uint16_t)-1 (even though that’s literally equal to 1)?

[Qfrost911]

I have reviewed the source code of imagehlp.CheckSumMappedFile, which was leaked in nt5src, it is indeed rounding up.

And the oldCheckSum , it was preserved in Windows source code also. Although it is useless now, I think it is reserved for future, because it is very easy to add checks internally, so I copied this part of the code.

[Qfrost911]

reCI

[thieta]

Looks good to me!

lld/COFF/Writer.cpp

603

I am a little surprised that we don't have a function for calculating this checksum already. We are not duplicating code with this?

[thieta]

Ah! Wait - we need tests for this as well!

[Qfrost911]

Oh well, I've tested a dozen samples using petool and they are all correct.

And I want to know, why this revision didn't pass the check in x64debian, I think my revision's format is correct.

[tonic]

Hello! Please hold off on approving this patch as its pending legal review. Thanks!

[Qfrost911]

To be honest, I didn't know what I should do.

Should I add some test sets to the vision?

[Qfrost911]

In D139184#3983197, @tonic wrote:

Hello! Please hold off on approving this patch as its pending legal review. Thanks!

Hi, did you mean should I keep waiting, or should I add some tests?

[aaron.ballman]

In D139184#4013034, @Qfrost911 wrote:

In D139184#3983197, @tonic wrote:

Hello! Please hold off on approving this patch as its pending legal review. Thanks!

Hi, did you mean should I keep waiting, or should I add some tests?

You should keep waiting -- the LLVM Foundation lawyers need some time to review the legal aspects of the contribution (which can take a fair amount of time, especially given the holiday season). That said, I'd be curious to hear what caused the foundation lawyer involvement in the first place given that PE32 is a publicly documented format (https://learn.microsoft.com/en-us/windows/win32/debug/pe-format); as a reviewer, it'd be helpful for me to know what to watch out for in this space.

[tonic]

Thanks so much for your patience. The concern was over the reference to leaked source code. This patch can be accepted assuming that no code was taken/copied from any copyrighted source.

[Qfrost911]

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.

[tonic]

In 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.

[aganea]

In D139184#4016073, @tonic wrote:

In 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, as documented here and here. We already have an implementation in libc, here, 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)

lld/COFF/Writer.cpp

617

@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.

[Qfrost911]

Thanks. This method is really concise and clear. If don't consider the possibility of future use, such as adding other verifications, it is advisable to remove this part of complex code.

[mstorsjo]

In D139184#4016943, @aganea wrote:

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.

Thanks for adding that context info!

The algorithm used for the PE checksum is essentially RFC1071, as documented here and here. We already have an implementation in libc, here, but I wouldn't touch or move it.

Thanks!

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

void Writer::writePEChecksum() {
[...]
  uint16_t *addr = (uint16_t *)buf;

I think this needs to use ulittle32_t *addr instead (as the images are written in that form anyway), so that we get the same checksum when running the linker on big-endian hosts. (I would expect that we do have some buildbot configs that run on big endian.)

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

Indeed, please add at least a mention of RFC1071! And we do need a testcase too, but I see that you're working on the prerequisites for that in D140555.

[Qfrost911]

Thanks.

But I have some questions. Why using "ulittle32_t" rather than "ulittle16_t".

And to be honest, I don't know how to achieve a test. It is very difficult to calc a checksum using lit tools.

[mstorsjo]

In D139184#4019123, @Qfrost911 wrote:

But I have some questions. Why using "ulittle32_t" rather than "ulittle16_t".

Sorry, that was a typo - it should indeed be ulittle16_t.

And to be honest, I don't know how to achieve a test. It is very difficult to calc a checksum using lit tools.

It should be quite straightforward.

Have a look at some existing test, e.g. lld/test/COFF/heap.test seems like a good example. First the test creates an object file from a stored yaml file (the exact contents doesn't matter much here). Then you would link one output without the /release flag, which should not set any checksum. After this, run llvm-readobj and inspect that the checksum in the output file is 0x0. (You may want to check for 0x0{{$}} to make sure that it doesn't accidentally match a substring like 0x01234.) Then run another link command with the /release flag, and check that it produces the expected checksum.

The exact value of the checksum in the test isn't that important - you don't need to verify the calculation of that separately. You've checked that the linker does the right thing in actual use - then just see what checksum value you get from the test input, and look for that as reference in the testcase. That way, we will notice if something changes (so that the checksum algorithm returns a different value than the expected one).

[Qfrost911]

Thank you very much.

But I still have some confusion. You mentioned if we post "/RELEASE", we need to compare the value read from llvm-readobj and an expected checksum. But how do I calc an expected checksum using lit tools? Adding RFC1071 in lit test? We cannot determine a unique checksum for each specific yaml, because some values in PE will be changed every time compilation. These values can impact the checksum.

[mstorsjo]

In D139184#4019268, @Qfrost911 wrote:

But I still have some confusion. You mentioned if we post "/RELEASE", we need to compare the value read from llvm-readobj and an expected checksum. But how do I calc an expected checksum using lit tools? Adding RFC1071 in lit test?

No, you don't need to reimplement it - like I said, it's enough to just check against a hardcoded test value.

We cannot determine a unique checksum for each specific yaml, because some values in PE will be changed every time compilation. These values can impact the checksum.

If that's the case, we should add flags that make the output deterministic.

There's a flag /Brepro which you can add, which changes the timestamp field into a deterministic hash-based timestamp. Or you can add /timestamp:0. With either of those flags in place, the output of e.g. the tests in the heap.test file I suggested as example, all are completely deterministic, producing exactly the same binary each time.

[Qfrost911]

Oh, I understand. Thank you very much. And I think I should wait reviewers accept D140555 so that I can use llvm-readobj to add tests.

[mstorsjo]

In D139184#4019296, @Qfrost911 wrote:

Oh, I understand. Thank you very much. And I think I should wait reviewers accept D140555 so that I can use llvm-readobj to add tests.

Yes, that would be best.

[Qfrost911]

add test

[aganea]

Looks good generally.

@tonic Does your veto still hold? Can we move ahead with this patch?

lld/COFF/Writer.cpp

620

Could you please expand the comment a bit? Something like « The PE checksum algorithm, implemented as suggested in RFC1071 »

[Qfrost911]

add commit

[tonic]

In D139184#4020932, @aganea wrote:

Looks good generally.

@tonic Does your veto still hold? Can we move ahead with this patch?

I have already provided feedback on the legal aspect and assuming those conditions are met (sounds like they are), then this patch is ok to go in.

[Qfrost911]

In D139184#4021208, @tonic wrote:

In D139184#4020932, @aganea wrote:

Looks good generally.

@tonic Does your veto still hold? Can we move ahead with this patch?

I have already provided feedback on the legal aspect and assuming those conditions are met (sounds like they are), then this patch is ok to go in.

Thanks. I deem that this patch includes a standard algorithm "RFC1071", and it is different between nt5src. Moreover, GCC achieves this feature, which means it is public.

Additionally, I want to know why the CI points my patch have some errors under "x64 debian failed". I have used clangd to format "lld/COFF/Driver.cpp" before I created the patch.

[thieta]

Looks like it's ready to go to me. Maybe take a look at the comments inline in the review and mark them as done or reply to them, there were some questions in there that I couldn't see any replies to.

lld/COFF/Config.h

struct Configuration {
   bool repro = false;
   bool swaprunCD = false;
   bool swaprunNet = false;
   bool thinLTOEmitImportsFiles;
   bool thinLTOIndexOnly;
   bool autoImport = false;
   bool pseudoRelocs = false;
   bool stdcallFixup = false;
+  bool writeCheckSum = false;
 };
 
 extern std::unique_ptr<Configuration> config;
 
 } // namespace lld::coff
 
 #endif

lld/COFF/Driver.cpp

case OPT_manifestdependency:
       config->manifestDependencies.insert(arg->getValue());
       break;
     case OPT_merge:
       parseMerge(arg->getValue());
       break;
     case OPT_nodefaultlib:
       config->noDefaultLibs.insert(doFindLib(arg->getValue()).lower());
       break;
+    case OPT_release:
+      config->writeCheckSum = true;
+      break;
     case OPT_section:
       parseSection(arg->getValue());
       break;
     case OPT_stack:
       parseNumbers(arg->getValue(), &config->stackReserve,
                    &config->stackCommit);
       break;
     case OPT_subsystem: {
void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   // addWinSysRootLibSearchPaths(), which is why they are in a separate loop.
   for (auto *arg : args.filtered(OPT_defaultlib))
     if (std::optional<StringRef> path = findLib(arg->getValue()))
       enqueuePath(*path, false, false);
   run();
   if (errorCount())
     return;
 
+  // Handle /RELEASE
+  if (args.hasArg(OPT_release))
+    config->writeCheckSum = true;
+  
   // Handle /safeseh, x86 only, on by default, except for mingw.
   if (config->machine == I386) {
     config->safeSEH = args.hasFlag(OPT_safeseh, OPT_safeseh_no, !config->mingw);
     config->noSEH = args.hasArg(OPT_noseh);
   }
 
   // Handle /functionpadmin
   for (auto *arg : args.filtered(OPT_functionpadmin, OPT_functionpadmin_opt))

lld/COFF/Options.td

def swaprun : P<"swaprun",
   "Comma-separated list of 'cd' or 'net'">;
 def swaprun_cd : F<"swaprun:cd">, Alias<swaprun>, AliasArgs<["cd"]>,
   HelpText<"Make loader run output binary from swap instead of from CD">;
 def swaprun_net : F<"swaprun:net">, Alias<swaprun>, AliasArgs<["net"]>,
   HelpText<"Make loader run output binary from swap instead of from network">;
 def verbose : F<"verbose">;
 def wholearchive_flag : F<"wholearchive">,
     HelpText<"Include all object files from all libraries">;
+def release : F<"release">,
+    HelpText<"Set the Checksum in the header of an PE file">;
 
 def force : F<"force">,
     HelpText<"Allow undefined and multiply defined symbols">;
 def force_unresolved : F<"force:unresolved">,
     HelpText<"Allow undefined symbols when creating executables">;
 def force_multiple : F<"force:multiple">,
     HelpText<"Allow multiply defined symbols when creating executables">;
 def force_multipleres : F<"force:multipleres">,

lld/COFF/Writer.cpp

private:
   void getSymbolsFromSections(ObjFile *file,
                               ArrayRef<SectionChunk *> symIdxChunks,
                               std::vector<Symbol *> &symbols);
   void maybeAddRVATable(SymbolRVASet tableSymbols, StringRef tableSym,
                         StringRef countSym, bool hasFlag=false);
   void setSectionPermissions();
   void writeSections();
   void writeBuildId();
+  void writePEChecksum();
   void sortSections();
   void sortExceptionTable();
   void sortCRTSectionChunks(std::vector<Chunk *> &chunks);
   void addSyntheticIdata();
   void fixPartialSectionChars(StringRef name, uint32_t chars);
   bool fixGnuImportChunks();
   void fixTlsAlignment();
   PartialSection *createPartialSection(StringRef name, uint32_t outChars);
while (true) {
     // Recalculate the layout for the whole image (and verify the ranges at
     // the start of the next round).
     assignAddresses();
 
     pass++;
   }
 }
 
+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;
+  ulittle16_t *addr = (ulittle16_t *)buf;
+
+  // The PE checksum algorithm, implemented as suggested in RFC1071
+  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;
+}
+
 // The main function of the writer.
 void Writer::run() {
   ScopedTimer t1(ctx.codeLayoutTimer);
 
   createImportTables();
   createSections();
   appendImportThunks();
   // Import thunks must be added before the Control Flow Guard tables are added.
if (!config->pdbPath.empty() && config->debug) {
     assert(buildId);
     createPDB(ctx, sectionTable, buildId->buildId);
   }
   writeBuildId();
 
   writeLLDMapFile(ctx);
   writeMapFile(ctx);
 
+  writePEChecksum();
+
   if (errorCount())
     return;
 
   ScopedTimer t2(ctx.outputCommitTimer);
   if (auto e = buffer->commit())
     fatal("failed to write output '" + buffer->getPath() +
           "': " + toString(std::move(e)));
 }

lld/test/COFF/checksum.test

+# RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj
+# RUN: lld-link /out:%t.exe /entry:main /timestamp:0 %p/Inputs/std32.lib %t.obj
+# RUN: llvm-readobj --file-headers %t.exe | FileCheck -check-prefix=CHECKSUM1 %s
+
+CHECKSUM1: CheckSum: 0x0
+
+# RUN: yaml2obj %p/Inputs/hello32.yaml -o %t.obj
+# RUN: lld-link /out:%t.exe /entry:main /timestamp:0 /RELEASE %p/Inputs/std32.lib %t.obj
+# RUN: llvm-readobj --file-headers %t.exe | FileCheck -check-prefix=CHECKSUM2 %s
+
+CHECKSUM2: CheckSum: 0x9196