[llvm] [Object][COFF][llvm-readobj] Add support for ARM64X dynamic relocations. (PR #97229)

Martin Storsjö via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 31 04:18:43 PDT 2024


================
@@ -800,6 +800,219 @@ Error COFFObjectFile::initLoadConfigPtr() {
     }
   }
 
+  // Interpret and validate dynamic relocations.
+  uint32_t DynamicRelocTableOffset = 0, DynamicRelocTableSection = 0;
+  if (is64()) {
+    auto Config = getLoadConfig64();
+    if (Config->Size >=
+        offsetof(coff_load_configuration64, DynamicValueRelocTableSection) +
+            sizeof(Config->DynamicValueRelocTableSection)) {
+      DynamicRelocTableSection = Config->DynamicValueRelocTableSection;
+      DynamicRelocTableOffset = Config->DynamicValueRelocTableOffset;
+    }
+  } else {
+    auto Config = getLoadConfig32();
+    if (Config->Size >=
+        offsetof(coff_load_configuration32, DynamicValueRelocTableSection) +
+            sizeof(Config->DynamicValueRelocTableSection)) {
+      DynamicRelocTableSection = Config->DynamicValueRelocTableSection;
+      DynamicRelocTableOffset = Config->DynamicValueRelocTableOffset;
+    }
+  }
+
+  Expected<const coff_section *> Section = getSection(DynamicRelocTableSection);
+  if (!Section)
+    return Section.takeError();
+  if (*Section) {
+    ArrayRef<uint8_t> Contents;
+    if (Error E = getSectionContents(*Section, Contents))
+      return E;
+
+    Contents = Contents.drop_front(DynamicRelocTableOffset);
+    if (Contents.size() < sizeof(coff_dynamic_reloc_table))
+      return createStringError(object_error::parse_failed,
+                               "Too large DynamicValueRelocTableOffset (" +
+                                   Twine(DynamicRelocTableOffset) + ")");
+
+    DynamicRelocTable =
+        reinterpret_cast<const coff_dynamic_reloc_table *>(Contents.data());
+
+    if (DynamicRelocTable->Version != 1 && DynamicRelocTable->Version != 2)
+      return createStringError(
+          object_error::parse_failed,
+          "Unsupported dynamic relocations table version (" +
+              Twine(DynamicRelocTable->Version) + ")");
+
+    Contents = Contents.drop_front(sizeof(*DynamicRelocTable));
+    if (DynamicRelocTable->Size > Contents.size())
+      return createStringError(object_error::parse_failed,
+                               "Indvalid dynamic relocations directory size (" +
+                                   Twine(DynamicRelocTable->Size) + ")");
+    Contents = Contents.take_front(DynamicRelocTable->Size);
+
+    while (!Contents.empty()) {
+      uint32_t DynRelocSize;
+      uint64_t Symbol;
+
+      if (DynamicRelocTable->Version == 1) {
+        if (is64()) {
+          if (Contents.size() < sizeof(coff_dynamic_relocation64))
+            return createStringError(
+                object_error::parse_failed,
+                "Unexpected end of dynamic relocations data");
+
+          auto DynReloc = reinterpret_cast<const coff_dynamic_relocation64 *>(
+              Contents.data());
+          Symbol = DynReloc->Symbol;
+          DynRelocSize = DynReloc->BaseRelocSize;
+          Contents = Contents.drop_front(sizeof(*DynReloc));
+        } else {
+          if (Contents.size() < sizeof(coff_dynamic_relocation32))
+            return createStringError(
+                object_error::parse_failed,
+                "Unexpected end of dynamic relocations data");
+
+          auto DynReloc = reinterpret_cast<const coff_dynamic_relocation32 *>(
+              Contents.data());
+          Symbol = DynReloc->Symbol;
+          DynRelocSize = DynReloc->BaseRelocSize;
+          Contents = Contents.drop_front(sizeof(*DynReloc));
+        }
+      } else {
+        if (is64()) {
+          if (Contents.size() < sizeof(coff_dynamic_relocation64_v2))
+            return createStringError(
+                object_error::parse_failed,
+                "Unexpected end of dynamic relocations data");
+
+          auto DynReloc =
+              reinterpret_cast<const coff_dynamic_relocation64_v2 *>(
+                  Contents.data());
+          if (DynReloc->HeaderSize < sizeof(*DynReloc) ||
+              Contents.size() < DynReloc->HeaderSize)
+            return createStringError(
+                object_error::parse_failed,
+                "Invalid dynamic relocation header size (" +
+                    Twine(DynReloc->HeaderSize) + ")");
+
+          Symbol = DynReloc->Symbol;
+          DynRelocSize = DynReloc->FixupInfoSize;
+          Contents = Contents.drop_front(DynReloc->HeaderSize);
+        } else {
+          if (Contents.size() < sizeof(coff_dynamic_relocation32_v2))
+            return createStringError(
+                object_error::parse_failed,
+                "Unexpected end of dynamic relocations data");
+
+          auto DynReloc =
+              reinterpret_cast<const coff_dynamic_relocation32_v2 *>(
+                  Contents.data());
+          if (DynReloc->HeaderSize < sizeof(*DynReloc) ||
+              Contents.size() < DynReloc->HeaderSize)
+            return createStringError(
+                object_error::parse_failed,
+                "Invalid dynamic relocation header size (" +
+                    Twine(DynReloc->HeaderSize) + ")");
+
+          Symbol = DynReloc->Symbol;
+          DynRelocSize = DynReloc->FixupInfoSize;
+          Contents = Contents.drop_front(DynReloc->HeaderSize);
+        }
+      }
+      if (DynRelocSize > Contents.size())
+        return createStringError(object_error::parse_failed,
+                                 "Too large dynamic relocation size (" +
+                                     Twine(DynRelocSize) + ")");
+
+      ArrayRef<uint8_t> RelocContents = Contents.take_front(DynRelocSize);
+      Contents = Contents.drop_front(DynRelocSize);
+
+      switch (Symbol) {
+      case COFF::IMAGE_DYNAMIC_RELOCATION_ARM64X:
+        while (!RelocContents.empty()) {
+          if (RelocContents.size() < sizeof(coff_base_reloc_block_header))
+            return createStringError(
+                object_error::parse_failed,
+                "Unexpected end of ARM64X relocations data");
+
+          auto Header = reinterpret_cast<const coff_base_reloc_block_header *>(
+              RelocContents.data());
+          if (Header->BlockSize <= sizeof(*Header))
+            return createStringError(object_error::parse_failed,
+                                     "ARM64X relocations block size (" +
+                                         Twine(Header->BlockSize) +
+                                         ") is too small");
+          if (Header->BlockSize % sizeof(uint32_t))
+            return createStringError(
+                object_error::parse_failed,
+                "Unaligned ARM64X relocations block size (" +
+                    Twine(Header->BlockSize) + ")");
+          if (Header->BlockSize > RelocContents.size())
+            return createStringError(object_error::parse_failed,
+                                     "ARM64X relocations block size (" +
+                                         Twine(Header->BlockSize) +
+                                         ") is too large");
+          if (Header->PageRVA & 0xfff)
+            return createStringError(object_error::parse_failed,
+                                     "Unaligned ARM64X relocations page RVA (" +
+                                         Twine(Header->PageRVA) + ")");
+
+          ArrayRef<uint16_t> Relocs(
+              reinterpret_cast<const uint16_t *>(RelocContents.data() +
+                                                 sizeof(*Header)),
+              (Header->BlockSize - sizeof(*Header)) / sizeof(uint16_t));
+          RelocContents = RelocContents.drop_front(Header->BlockSize);
+
+          while (!Relocs.empty()) {
+            if (!Relocs[0]) {
+              if (Relocs.size() != 1)
+                return createStringError(
+                    object_error::parse_failed,
+                    "Unexpected ARM64X relocations terminator");
+              break;
+            }
+
+            uint16_t Arg = Relocs[0] >> 14, RelocSize = 1, Size;
+            switch ((Relocs[0] >> 12) & 3) {
+            case COFF::IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL:
+              Size = 1 << Arg;
+              break;
+            case COFF::IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE:
+              if (!Arg)
+                return createStringError(
+                    object_error::parse_failed,
+                    "Invalid ARM64X relocation value size (0)");
+              Size = 1 << Arg;
+              RelocSize += Size / sizeof(Relocs[0]);
+              break;
+            case COFF::IMAGE_DVRT_ARM64X_FIXUP_TYPE_DELTA:
+              ++RelocSize;
+              Size = sizeof(uint32_t);
+              break;
+            default:
+              return createStringError(object_error::parse_failed,
+                                       "Invalid relocation type");
+            }
+            if (Header->PageRVA) {
+              uint64_t IntPtr;
+              uint16_t Offset = Relocs[0] & 0xfff;
+              if (Offset % Size)
+                return createStringError(object_error::parse_failed,
+                                         "Unaligned ARM64X relocation RVA (" +
+                                             Twine(Header->PageRVA + Offset) +
+                                             ")");
+              if (Error E = getRvaPtr(Header->PageRVA + Offset + Size, IntPtr,
----------------
mstorsjo wrote:

Yes, that commit makes it a little bit more palatable.

I guess part of this comes from the overall design of these classes; that we normally do all checking upfront, and then we just return data that is supposed to be known safe. For this kind of data structure, which is variable length in two layers, the verification requires doing the whole iteration first once upfront, and then later via the iterators. If it would be possible to return such validation errors from the iterators, instead of upfront, I feel we would be able to get rid of the duplication entirely.

Then again - I'm not sure what others, with more experience with our `ObjectFile` classes, think about this - @efriedma-quic, any opinions?

Alternatively, would it be possible to collapse it even further, into basically what the caller would do, e.g. something like this:
```
    for (auto reloc : dynamic_relocs()) {
      switch (reloc.getType()) {
      case COFF::IMAGE_DYNAMIC_RELOCATION_ARM64X: {
        for (auto Arm64XReloc : reloc.arm64x_relocs())
          getRvaPtr(Arm64XReloc.getRVA());
```

The iterator interfaces themselves don't expose many opportunities for validation, but would it be possible to add e.g. a `Error validate();` method on the iterators, which would encapsulate all this (checking validity of the RVAs, checking for alignment issues etc), so that the check could boil down to just this:
```
    for (auto reloc : dynamic_relocs())
        if (Error e = reloc.validate())
            return e;
```

As I'm a little unsure whether I'm leading you in the right direction or just diverging from the common LLVM ObjectFile code style, I'd wait to see if @efriedma-quic agrees or disagrees with my suggestions here.

https://github.com/llvm/llvm-project/pull/97229


More information about the llvm-commits mailing list