[WebAssembly] Add validation to reloc section

[ncw]

Summary

We now check relocations offsets are within range, and the relocation index is valid.

Also updated tests which contained invalid Wasm files that were previously not checked.

---

Could this be controversial?!? I know some people advocate for "minimal" correctness checks in LLVM, since the linker reads in hundreds of object files and we don't want to do expensive validation for each one.

These checks are pretty lightweight though, and should handily catch any bugs we might have.

[sbc100]

include/llvm/Object/Wasm.h

208

I find these method names a little confusing. Perhaps the code will be clearer just to inline them? (or perhaps drop the final Index from the name)?

lib/Object/WasmObjectFile.cpp

650

I think one variable per line makes this more readable (although I don't see anything in the llvm style it certainly seems to be the defacto style in the code I've been looking at).

Maybe call the first one LastRelocOffset?

709

Hmm, you can't do this at parse time because the symbol table doesn't exist yet?

In that case i think we need to specify that the reloc sections need to come after the linking metadata section, since they depend on the symbol table.

test/ObjectYAML/wasm/code_section.yaml

53

What is this adding the to the test? Presumably you might want to add new tests to check the failure cases?

test/ObjectYAML/wasm/data_section.yaml

35

Ditto

[ncw]

include/llvm/Object/Wasm.h

208

Will do.

lib/Object/WasmObjectFile.cpp

709

Hmm, that only works if the reloc sections come together at the end of the file.

• CODE must come before DATA
• Symbol table must come after DATA (since otherwise the Data symbol indexes can't be validated as it's read in)
• Relocs have to come at the end of the file therefore

I can make that change - it'll be a big tedious reordering of all the test output, but should be fairly straightforward.

This PR can go on hold then until it's that's done.

test/ObjectYAML/wasm/code_section.yaml

53

It's adding to the test since the test contains a file with relocations but no symbol table (so it's an invalid file).

You're right, I could add tests for exercising our handling of bad Wasm files... but we currently have very very few tests for bad Wasm files. What's the policy - is it our intention to have tests for all the various cases of illegal Wasm files?

I guess we would ideally, I'll add a test for the new error messages I've put in.

[sbc100]

test/ObjectYAML/wasm/code_section.yaml

53

Oh I see. You mean that added these validations caused these tests to start failing? In that case yes, I think these test changes makes sense to be part of this CL. Perhaps mention that in the description.

[ncw]

Rebased on top of D43940; it's now simpler if we force the reloc section to come after the symtab.

[ncw]

Rebased

[sbc100]

(might want to clean up the description a little when committing).

include/llvm/Object/Wasm.h

public:
   SubtargetFeatures getFeatures() const override;
   bool isRelocatableObject() const override;
 
 private:
   bool isValidFunctionIndex(uint32_t Index) const;
   bool isDefinedFunctionIndex(uint32_t Index) const;
   bool isValidGlobalIndex(uint32_t Index) const;
   bool isDefinedGlobalIndex(uint32_t Index) const;
-  bool isValidFunctionSymbolIndex(uint32_t Index) const;
+  bool isValidFunctionSymbol(uint32_t Index) const;
+  bool isValidGlobalSymbol(uint32_t Index) const;
+  bool isValidDataSymbol(uint32_t Index) const;
   wasm::WasmFunction &getDefinedFunction(uint32_t Index);
   wasm::WasmGlobal &getDefinedGlobal(uint32_t Index);
 
   const WasmSection &getWasmSection(DataRefImpl Ref) const;
   const wasm::WasmRelocation &getWasmRelocation(DataRefImpl Ref) const;
 
   WasmSection* findCustomSectionByName(StringRef Name);
   WasmSection* findSectionByType(uint32_t Type);

lib/Object/WasmObjectFile.cpp

while (Ptr < End) {
     }
     case wasm::WASM_INIT_FUNCS: {
       uint32_t Count = readVaruint32(Ptr);
       LinkingData.InitFunctions.reserve(Count);
       for (uint32_t i = 0; i < Count; i++) {
         wasm::WasmInitFunc Init;
         Init.Priority = readVaruint32(Ptr);
         Init.Symbol = readVaruint32(Ptr);
-        if (!isValidFunctionSymbolIndex(Init.Symbol))
+        if (!isValidFunctionSymbol(Init.Symbol))
           return make_error<GenericBinaryError>("Invalid function symbol: " +
                                                     Twine(Init.Symbol),
                                                 object_error::parse_failed);
         LinkingData.InitFunctions.emplace_back(Init);
       }
       break;
     }
     case wasm::WASM_COMDAT_INFO:
if (SectionCode == wasm::WASM_SEC_CUSTOM) {
     Section = findCustomSectionByName(Name);
   } else {
     Section = findSectionByType(SectionCode);
   }
   if (!Section)
     return make_error<GenericBinaryError>("Invalid section code",
                                           object_error::parse_failed);
   uint32_t RelocCount = readVaruint32(Ptr);
+  uint32_t LastOffset = 0;
+  uint32_t EndOffset = Section->Content.size();
   while (RelocCount--) {
-    wasm::WasmRelocation Reloc;
-    memset(&Reloc, 0, sizeof(Reloc));
+    wasm::WasmRelocation Reloc = {};
     Reloc.Type = readVaruint32(Ptr);
     Reloc.Offset = readVaruint32(Ptr);
     Reloc.Index = readVaruint32(Ptr);
     switch (Reloc.Type) {
     case wasm::R_WEBASSEMBLY_FUNCTION_INDEX_LEB:
     case wasm::R_WEBASSEMBLY_TABLE_INDEX_SLEB:
     case wasm::R_WEBASSEMBLY_TABLE_INDEX_I32:
+      if (!isValidFunctionSymbol(Reloc.Index))
+        return make_error<GenericBinaryError>("Bad relocation function index",
+                                              object_error::parse_failed);
+      break;
     case wasm::R_WEBASSEMBLY_TYPE_INDEX_LEB:
+      if (Reloc.Index >= Signatures.size())
+        return make_error<GenericBinaryError>("Bad relocation type index",
+                                              object_error::parse_failed);
+      break;
     case wasm::R_WEBASSEMBLY_GLOBAL_INDEX_LEB:
+      if (!isValidGlobalSymbol(Reloc.Index))
+        return make_error<GenericBinaryError>("Bad relocation global index",
+                                              object_error::parse_failed);
       break;
     case wasm::R_WEBASSEMBLY_MEMORY_ADDR_LEB:
     case wasm::R_WEBASSEMBLY_MEMORY_ADDR_SLEB:
     case wasm::R_WEBASSEMBLY_MEMORY_ADDR_I32:
+      if (!isValidDataSymbol(Reloc.Index))
+        return make_error<GenericBinaryError>("Bad relocation data index",
+                                              object_error::parse_failed);
       Reloc.Addend = readVarint32(Ptr);
       break;
     default:
       return make_error<GenericBinaryError>("Bad relocation type: " +
                                                 Twine(Reloc.Type),
                                             object_error::parse_failed);
     }
+
+    // Relocations must fit inside the section, and must appear in order.  They
+    // also shouldn't overlap a function/element boundary, but we don't bother
+    // to check that.
+    uint64_t Size = 5;
+    if (Reloc.Type == wasm::R_WEBASSEMBLY_TABLE_INDEX_I32 ||
+        Reloc.Type == wasm::R_WEBASSEMBLY_MEMORY_ADDR_I32)
+      Size = 4;
+    if (Reloc.Offset < LastOffset || Reloc.Offset + Size > EndOffset)
+      return make_error<GenericBinaryError>("Bad relocation offset",
+                                            object_error::parse_failed);
+    LastOffset = Reloc.Offset;
+
     Section->Relocations.push_back(Reloc);
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Reloc section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
 }
 
 Error WasmObjectFile::parseCustomSection(WasmSection &Sec,
                                          const uint8_t *Ptr, const uint8_t *End) {
   Ptr = Sec.Name.bytes_end();
   if (Sec.Name == "name") {
     if (Error Err = parseNameSection(Ptr, End))
       return Err;
   } else if (Sec.Name == "linking") {
     if (Error Err = parseLinkingSection(Ptr, End))
       return Err;

 bool WasmObjectFile::isValidGlobalIndex(uint32_t Index) const {
   return Index < NumImportedGlobals + Globals.size();
 }
 
 bool WasmObjectFile::isDefinedGlobalIndex(uint32_t Index) const {
   return Index >= NumImportedGlobals && isValidGlobalIndex(Index);
 }
 
-bool WasmObjectFile::isValidFunctionSymbolIndex(uint32_t Index) const {
+bool WasmObjectFile::isValidFunctionSymbol(uint32_t Index) const {
   return Index < Symbols.size() && Symbols[Index].isTypeFunction();
 }
 
+bool WasmObjectFile::isValidGlobalSymbol(uint32_t Index) const {
+  return Index < Symbols.size() && Symbols[Index].isTypeGlobal();
+}
+
+bool WasmObjectFile::isValidDataSymbol(uint32_t Index) const {
+  return Index < Symbols.size() && Symbols[Index].isTypeData();
+}
+
 wasm::WasmFunction &WasmObjectFile::getDefinedFunction(uint32_t Index) {
   assert(isDefinedFunctionIndex(Index));
   return Functions[Index - NumImportedFunctions];
 }
 
 wasm::WasmGlobal &WasmObjectFile::getDefinedGlobal(uint32_t Index) {
   assert(isDefinedGlobalIndex(Index));
   return Globals[Index - NumImportedGlobals];

test/ObjectYAML/wasm/code_section.yaml

Functions:
          - Type:            I32
            Count:           3
         Body:            418080808000210020002101200111808080800000210220020F0B
       - Index:              1
         Locals:
          - Type:            I32
            Count:           1
         Body:            108180808000210020000F0B
+  - Type:            CUSTOM
+    Name:            linking
+    SymbolTable:
+      - Index:           0
+        Kind:            FUNCTION
+        Name:            func1
+        Flags:           [  ]
+        Function:        0
+      - Index:           1
+        Kind:            FUNCTION
+        Name:            func2
+        Flags:           [  ]
+        Function:        1
 ...
 # CHECK: --- !WASM
 # CHECK: FileHeader:
 # CHECK:   Version:           0x00000001
 # CHECK: Sections:
 # CHECK:  - Type:            TYPE
 # CHECK:    Signatures:
 # CHECK:      - Index:           0
 # CHECK:        ReturnType:      F32

test/ObjectYAML/wasm/data_section.yaml

Relocations:
       - Type:            R_WEBASSEMBLY_MEMORY_ADDR_I32
         Index:           0
         Offset:          0x00000006
         Addend:          8
       - Type:            R_WEBASSEMBLY_MEMORY_ADDR_I32
         Index:           0
         Offset:          0x00000006
         Addend:          -6
+  - Type:            CUSTOM
+    Name:            linking
+    SymbolTable:
+      - Index:           0
+        Kind:            DATA
+        Name:            dataSymbol
+        Flags:           [  ]
+        Segment:         0
+        Offset:          0
+        Size:            4
 ...
 # CHECK: --- !WASM
 # CHECK: FileHeader:
 # CHECK:   Version:           0x00000001
 # CHECK: Sections:
 # CHECK:   - Type:            DATA
 # CHECK-NEXT:     Relocations:
 # CHECK-NEXT:      - Type:            R_WEBASSEMBLY_MEMORY_ADDR_I32
 # CHECK-NEXT:        Index:           0
 # CHECK-NEXT:        Offset:          0x00000006
 # CHECK-NEXT:        Addend:          8
 # CHECK-NEXT:      - Type:            R_WEBASSEMBLY_MEMORY_ADDR_I32
 # CHECK-NEXT:        Index:           0
 # CHECK-NEXT:        Offset:          0x00000006
 # CHECK-NEXT:        Addend:          -6
 # CHECK-NEXT:     Segments:
 # CHECK-NEXT:       - SectionOffset:   6
 # CHECK-NEXT:         MemoryIndex:     0
 # CHECK-NEXT:         Offset:
 # CHECK-NEXT:           Opcode:          I32_CONST
 # CHECK-NEXT:           Value:           4
 # CHECK-NEXT:         Content:         '10001000'
-# CHECK-NEXT: ...
+# CHECK-NEXT:   - Type:            CUSTOM