fix(relinker): ignore direct branch targets inside an instruction past its prefixes

core/relinker/codegen/src/Amd64OnlyConverter.cpp

@@ -2,6 +2,7 @@
 #include <codegen/IInstructionScanner.hpp>
 #include <codegen/CodegenException.hpp>
 #include <codegen/x86/Amd64OnlySubstitutionTable.hpp>
+#include <codegen/x86/DecodedInstruction.hpp>
 #include <codegen/x86/X64InstructionDecoder.hpp>
 #include <codegen/x86/X64OpcodeConstants.hpp>
 #include <codegen/x86/IAmd64OnlyInstructionMatcher.hpp>
@@ -260,14 +261,23 @@ std::set<std::uint64_t> Amd64OnlyConverter::_collectBranchTargets(
         const auto match = std::lower_bound(matches.begin(), matches.end(), offset, [](const InstructionMatch& item, const std::uint64_t value) { return item.Offset < value; });
         return match != matches.end() && match->Offset == offset;
     };
+    const auto pastPrefixes = [&](const std::uint64_t address) {
+        const auto offset = address - ph.MappedAddress;
+        const auto after = std::upper_bound(matches.begin(), matches.end(), offset, [](const std::uint64_t value, const InstructionMatch& item) { return value < item.Offset; });
+        if (after == matches.begin())
+            return false;
+        const auto& match = *std::prev(after);
+        const auto inside = offset - match.Offset;
+        return inside != 0 && inside < match.Length && inside > DecodedInstruction{seg.data() + match.Offset, match.Length}.OpcodeOffset();
+    };
     const X64InstructionDecoder decoder;
     std::set<std::uint64_t> targets;
     for (const auto& match : matches) {
         const auto info = decoder.DecodeInstruction(seg.data() + match.Offset, match.Length);
         const auto next = static_cast<std::int64_t>(ph.MappedAddress + match.Offset + info.Length);
         if (info.HasBranchTarget && !info.HasRipRelativeDisp) {
             const auto target = next + info.BranchDisp;
-            if (target >= 0)
+            if (target >= 0 && !pastPrefixes(static_cast<std::uint64_t>(target)))
                 targets.insert(static_cast<std::uint64_t>(target));
         } else if (info.HasRipRelativeDisp && info.Opcode == kLea) {
             std::int32_t displacement;

core/relinker/codegen/tests/Amd64OnlyConverterTests.cpp

@@ -885,6 +885,14 @@ void converterWideStoreWindows() {
     expected.push_back(0xC3);
     require(returnIsland.Trampolines.size() == 2 && returnIsland.Trampolines[0].Offset == 0x20B && body(returnIsland.Trampolines[0]) == expected, "Instructions moved for an island that end in a return jump back after it");
 
+    const auto intoStore = convert(withStore({0x73, 0x01}, {0xC3}));
+    expected = halves;
+    expected.push_back(0xC3);
+    require(intoStore.Trampolines.size() == 1 && intoStore.Trampolines[0].Offset == 0x202 && intoStore.Trampolines[0].Length == 5 && body(intoStore.Trampolines[0]) == expected, "A branch into the middle of a 256-bit store, where no scanned instruction starts, kept the store");
+
+    const auto intoFollower = convert(withStore({0x73, 0x06}, {0x48, 0x83, 0xC1, 0x01, 0xC3}));
+    require(intoFollower.Trampolines.size() == 1 && intoFollower.Trampolines[0].Offset == 0x202 && intoFollower.Trampolines[0].Length == 8, "A branch into the middle of an instruction moved with a 256-bit store kept the store");
+
     const auto afterBranch = convert(withStore({0x75, 0x10}, {0xFF, 0xD0}));
     expected = {0x0F, 0x85, 0, 0, 0, 0};
     expected.insert(expected.end(), halves.begin(), halves.end());
@@ -1054,10 +1062,15 @@ void converterRosettaScalarCompare() {
     require(site.Offset == 0x20C && site.Length == 5 && site.ReturnBranchOffset == 5 && Bytes(site.Body.begin(), site.Body.begin() + 5) == Bytes(text.begin() + 12, text.begin() + 17) && Bytes(site.Body.begin() + 5, site.Body.begin() + 10) == Bytes{0xE9, 0, 0, 0, 0} && site.Relocations.empty(), "The compare stub does not run the compare unchanged and return");
     require(result.Bytes == file, "Moving the compare changed the file image");
     auto branchInside = file;
-    const Bytes jumped = {0xEB, 0x02, 0xC5, 0xEA, 0xC2, 0xCA, 0x03, 0xC3};
+    const Bytes jumped = {0xEB, 0x01, 0xF3, 0x0F, 0xC2, 0xCA, 0x03, 0xC3};
     std::copy(jumped.begin(), jumped.end(), branchInside.begin() + 0x200);
     const auto kept = converter->Convert(branchInside, {segmentHeader(jumped.size())});
-    require(kept.Trampolines.empty() && kept.KeptCount == 1 && kept.Reports.size() == 1 && kept.Reports[0].Lowering == Codegen::Amd64OnlyLowering::Kept && kept.Bytes == branchInside, "A branch into the compare did not keep it native");
+    require(kept.Trampolines.empty() && kept.KeptCount == 1 && kept.Reports.size() == 1 && kept.Reports[0].Lowering == Codegen::Amd64OnlyLowering::Kept && kept.Bytes == branchInside, "A branch past the prefix of the compare did not keep it native");
+    auto branchIntoVex = file;
+    const Bytes vexJumped = {0xEB, 0x02, 0xC5, 0xEA, 0xC2, 0xCA, 0x03, 0xC3};
+    std::copy(vexJumped.begin(), vexJumped.end(), branchIntoVex.begin() + 0x200);
+    const auto moved = converter->Convert(branchIntoVex, {segmentHeader(vexJumped.size())});
+    require(moved.Trampolines.size() == 1 && moved.Trampolines[0].Offset == 0x202 && moved.KeptCount == 0, "A branch past the VEX prefix of the compare, where no instruction starts, kept the compare native");
     auto ripSource = file;
     const Bytes ripText = {0xC5, 0xEA, 0xC2, 0x0D, 0x00, 0x01, 0x00, 0x00, 0x03, 0xC3};
     std::copy(ripText.begin(), ripText.end(), ripSource.begin() + 0x200);

docs/dev/debt/relinker.md

@@ -13,6 +13,7 @@
 - The [instruction decoder](../../../core/relinker/codegen/src/x86/X64InstructionDecoder.cpp) reads a 16-bit displacement for `66 E8`, `66 E9` and `66 0F 80`-`8F` without REX.W, as AMD processors, the console included, do (binutils `objdump -M amd64`; REX.W selects the 32-bit form). AMD also truncates the target of every near relative branch with that prefix, rel8, LOOP and JRCXZ forms included, to 16 bits. Instruction metadata marks these branches as truncated and reports no branch target for them, and stubs refuse to move them. Compilers do not emit these forms.
 - `--to-intel` does not lower RDPRU/MCOMMIT; the [matcher](../../../core/relinker/codegen/src/x86/Amd64OnlyInstructionMatcher.cpp) fails the relink instead.
 - The length-changing path of the [instruction rewriter](../../../core/relinker/codegen/src/x86/X64InstructionRewriter.cpp) is not used by the [converter](../../../core/relinker/codegen/src/Amd64OnlyConverter.cpp): it does not adjust data-to-code references (relocations, FDEs, jump tables) or segment sizes, so every substitution keeps the instruction length.
+- The [converter](../../../core/relinker/codegen/src/Amd64OnlyConverter.cpp) takes the branch targets that keep an instruction in place from a linear scan of the code segment, so data in that segment decodes as branches too: OpenSSL's x86-64 assembly keeps strings such as "SHA1 block transform for x86_64, CRYPTOGAMS by <appro@openssl.org>" in its code, and in SuperTuxKart (PPSA69421) and Mortalfin (PPSA99141) the `jae` read from "sf" points into the VEX prefix of a 256-bit store. A direct branch target inside a scanned instruction counts only where every byte before it is a legacy prefix or REX, which a real branch can skip (a `lock` prefix, for example); a target in a VEX prefix, opcode or operand is ignored, as a jump-table entry that starts no instruction already is. A real branch to such a target, overlapping instructions as hand-written code can, is not seen, and the instruction it enters can be moved.
 - A `PT_SCE_RELRO` segment that no `PT_LOAD` covers, the PS4 layout, is [loaded](../../../core/relinker/relinker/src/parsing/ElfReader.cpp) as a read-write `PT_LOAD` and stays writable after relocation; FreeBSD rtld makes `PT_GNU_RELRO` read-only once relocation is done, and what the console does with `PT_SCE_RELRO` is not checked.
 - In Windows output, a guest instruction that addresses memory through GS becomes `UD2` and NOP padding of its own length ([TLS builder](../../../core/relinker/elfpatcher/src/windows/WindowsTlsBuilder.cpp)): the host owns GS, and the guest GS base that FreeBSD's `sysarch(AMD64_SET_GSBASE)` would set is not emulated. A converted module runs until such an instruction is reached, which then faults. Prospero Win (PPSA99995) has six in its Wine `ntdll`.
 - In executables with a `PT_SCE_DYNLIBDATA` segment, the PS4 layout, the executable segment also holds read-only data after the code (CUSA00900: 37 MiB after its last function). The [relinker pipeline](../../../core/relinker/relinker/src/pipeline/RelinkerPipeline.cpp) therefore scans for syscalls and import call sites only up to the end of the PLT that follows the last function of the unwind table; code after the PLT without an unwind entry is not scanned. `--to-intel` and the `unused-filter=1` and `unused-filter=2` analyses are not limited this way.