[JITLink] Add fixup value range check

[StephenFan]

Summary

This patch makes jitlink to report an out of range error when the fixup value out of range

[lhames]

LGTM. Thanks Stephen!

[jrtc27]

This was clearly not done whilst referencing LLD's implementation as it's buggy. Please fix.

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

199–201

You need to check that Value + 0x800 is a 32-bit signed integer, not that Value is a 32-bit unsigned integer. LUI sign-extends, hence checking it as a signed integer, and the + 0x800 needs to be added to account for the slight asymmetry resulting from the ADDI/etc's sign-extension of the LO12.

212

LO12 should never be range checked, it's a waste of time as the HI20 half will also range check.

233

As with HI20, this needs to check Value + 0x800, not Value. But this at least already checks it as a signed integer rather than an unsigned integer.

251

As with LO12, these are a waste of time

[lhames]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

212

If LO12/HI20 relocations are always paired then it seems reasonable to elide the range check here, but we'd need a check that both elements of the pair are present in the input object: JITLink's policy is to assume that objects may be malformed / maliciously crafted and always check.

That needn't be addressed straight away, but we should at least add a FIXME for it.

[StephenFan]

Address @jrtc27's comments

[lhames]

The code should be clang-formatted. Otherwise LGTM.

[StephenFan]

Clang-format and delete unused function

[StephenFan]

clang-format

[StephenFan]

rebase

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

static Expected<const Edge &> getRISCVPCRelHi20(const Edge &E) {
   return make_error<JITLinkError>(
       "No HI20 PCREL relocation type be found for LO12 PCREL relocation type");
 }
 
 static uint32_t extractBits(uint32_t Num, unsigned Low, unsigned Size) {
   return (Num & (((1ULL << (Size + 1)) - 1) << Low)) >> Low;
 }
 
+static inline bool isInRangeForImmS32(int64_t Value) {
+  return (Value >= std::numeric_limits<int32_t>::min() &&
+          Value <= std::numeric_limits<int32_t>::max());
+}
+
 class ELFJITLinker_riscv : public JITLinker<ELFJITLinker_riscv> {
   friend class JITLinker<ELFJITLinker_riscv>;
 
 public:
   ELFJITLinker_riscv(std::unique_ptr<JITLinkContext> Ctx,
                      std::unique_ptr<LinkGraph> G, PassConfiguration PassConfig)
       : JITLinker(std::move(Ctx), std::move(G), std::move(PassConfig)) {}
 
Error applyFixup(LinkGraph &G, Block &B, const Edge &E) const {
     }
     case R_RISCV_64: {
       int64_t Value = (E.getTarget().getAddress() + E.getAddend()).getValue();
       *(little64_t *)FixupPtr = static_cast<uint64_t>(Value);
       break;
     }
     case R_RISCV_HI20: {
       int64_t Value = (E.getTarget().getAddress() + E.getAddend()).getValue();
-      int32_t Hi = (Value + 0x800) & 0xFFFFF000;
+      int64_t Hi = Value + 0x800;
+      if (LLVM_UNLIKELY(!isInRangeForImmS32(Hi)))
+        return makeTargetOutOfRangeError(G, B, E);
       uint32_t RawInstr = *(little32_t *)FixupPtr;
-      *(little32_t *)FixupPtr = (RawInstr & 0xFFF) | static_cast<uint32_t>(Hi);
+      *(little32_t *)FixupPtr =
+          (RawInstr & 0xFFF) | (static_cast<uint32_t>(Hi & 0xFFFFF000));
       break;
     }
     case R_RISCV_LO12_I: {
+      // FIXME: We assume that R_RISCV_HI20 is present in object code and pairs
+      // with current relocation R_RISCV_LO12_I. So here may need a check.
       int64_t Value = (E.getTarget().getAddress() + E.getAddend()).getValue();
       int32_t Lo = Value & 0xFFF;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
       *(little32_t *)FixupPtr =
           (RawInstr & 0xFFFFF) | (static_cast<uint32_t>(Lo & 0xFFF) << 20);
       break;
     }
     case R_RISCV_CALL: {
       int64_t Value = E.getTarget().getAddress() + E.getAddend() - FixupAddress;
-      int32_t Hi = (Value + 0x800) & 0xFFFFF000;
+      int64_t Hi = Value + 0x800;
+      if (LLVM_UNLIKELY(!isInRangeForImmS32(Hi)))
+        return makeTargetOutOfRangeError(G, B, E);
       int32_t Lo = Value & 0xFFF;
       uint32_t RawInstrAuipc = *(little32_t *)FixupPtr;
       uint32_t RawInstrJalr = *(little32_t *)(FixupPtr + 4);
-      *(little32_t *)FixupPtr = RawInstrAuipc | static_cast<uint32_t>(Hi);
+      *(little32_t *)FixupPtr =
+          RawInstrAuipc | (static_cast<uint32_t>(Hi & 0xFFFFF000));
       *(little32_t *)(FixupPtr + 4) =
           RawInstrJalr | (static_cast<uint32_t>(Lo) << 20);
       break;
     }
     case R_RISCV_PCREL_HI20: {
       int64_t Value = E.getTarget().getAddress() + E.getAddend() - FixupAddress;
-      int32_t Hi = (Value + 0x800) & 0xFFFFF000;
+      int64_t Hi = Value + 0x800;
+      if (LLVM_UNLIKELY(!isInRangeForImmS32(Hi)))
+        return makeTargetOutOfRangeError(G, B, E);
       uint32_t RawInstr = *(little32_t *)FixupPtr;
-      *(little32_t *)FixupPtr = (RawInstr & 0xFFF) | static_cast<uint32_t>(Hi);
+      *(little32_t *)FixupPtr =
+          (RawInstr & 0xFFF) | (static_cast<uint32_t>(Hi & 0xFFFFF000));
       break;
     }
     case R_RISCV_PCREL_LO12_I: {
+      // FIXME: We assume that R_RISCV_PCREL_HI20 is present in object code and
+      // pairs with current relocation R_RISCV_PCREL_LO12_I. So here may need a
+      // check.
       auto RelHI20 = getRISCVPCRelHi20(E);
       if (!RelHI20)
         return RelHI20.takeError();
       int64_t Value = RelHI20->getTarget().getAddress() +
                       RelHI20->getAddend() - E.getTarget().getAddress();
       int64_t Lo = Value & 0xFFF;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
       *(little32_t *)FixupPtr =
           (RawInstr & 0xFFFFF) | (static_cast<uint32_t>(Lo & 0xFFF) << 20);
       break;
     }
     case R_RISCV_PCREL_LO12_S: {
+      // FIXME: We assume that R_RISCV_PCREL_HI20 is present in object code and
+      // pairs with current relocation R_RISCV_PCREL_LO12_S. So here may need a
+      // check.
       auto RelHI20 = getRISCVPCRelHi20(E);
       int64_t Value = RelHI20->getTarget().getAddress() +
                       RelHI20->getAddend() - E.getTarget().getAddress();
       int64_t Lo = Value & 0xFFF;
       uint32_t Imm31_25 = extractBits(Lo, 5, 7) << 25;
       uint32_t Imm11_7 = extractBits(Lo, 0, 5) << 7;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
 

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_abs_reloc.s

 # RUN: rm -rf %t && mkdir -p %t
 # RUN: llvm-mc -triple=riscv64 -filetype=obj \
 # RUN:     -o %t/elf_riscv64_non_pc_indirect_reloc.o %s
 # RUN: llvm-mc -triple=riscv32 -filetype=obj \
 # RUN:     -o %t/elf_riscv32_non_pc_indirect_reloc.o %s
 # RUN: llvm-jitlink -noexec \
-# RUN:     -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
-# RUN:     -define-abs external_data=0xfff10000 \
+# RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
+# RUN:     -define-abs external_data=0x1ff10000 \
 # RUN:     -check %s %t/elf_riscv64_non_pc_indirect_reloc.o
 # RUN: llvm-jitlink -noexec \
-# RUN:     -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
-# RUN:     -define-abs external_data=0xfff10000 \
+# RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
+# RUN:     -define-abs external_data=0x1ff10000 \
 # RUN:     -check %s %t/elf_riscv32_non_pc_indirect_reloc.o
 #
 
         .text
         .file   "testcase.c"
 
 # Empty main entry point.
         .globl  main

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

 # RUN: rm -rf %t && mkdir -p %t
 # RUN: llvm-mc -triple=riscv64 -position-independent -filetype=obj \
 # RUN:     -o %t/elf_riscv64_sm_pic_reloc.o %s
 # RUN: llvm-mc -triple=riscv32 -position-independent -filetype=obj \
 # RUN:     -o %t/elf_riscv32_sm_pic_reloc.o %s
 # RUN: llvm-jitlink -noexec \
-# RUN:     -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
 # RUN:     -define-abs external_func=0x1 -define-abs external_data=0x2 \
 # RUN:     -check %s %t/elf_riscv64_sm_pic_reloc.o
 # RUN: llvm-jitlink -noexec \
-# RUN:     -slab-allocate 100Kb -slab-address 0xfff00000 -slab-page-size 4096 \
+# RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
 # RUN:     -define-abs external_func=0x1 -define-abs external_data=0x2 \
 # RUN:     -check %s %t/elf_riscv32_sm_pic_reloc.o
 #
 # Test ELF small/PIC relocations
 
         .text
         .file   "testcase.c"