Hi Simon,
I have updated the commit message with these details. I also documented the original corruption-check purpose of the ordering test. The new lower and upper bounds on the relocation location, together with the existing check on the pointed-to value, reject entries outside the image. An in-range corrupt entry cannot be distinguished from a valid unordered one, as you noted. Thanks, Jerome On 28/10/2026 12:24, Simon Glass wrote: > Hi Jerome, > > On 2026-10-27T16:02:16, Jerome Forissier <[email protected]> wrote: >> x86: Handle unordered runtime relocations >> >> Runtime relocation entries are not guaranteed to be sorted by target >> address. The existing code skips every entry whose address is below the >> previous one, leaving it unrelocated. > > Please can you say which toolchain or linker options give unsorted > entries, and what goes wrong when it happens? My understanding is that > GNU ld with combreloc (the default) sorts the RELATIVE relocations by > offset, so it would help to know where this was seen, e.g. lld, a > newer binutils or -z nocombreloc The CI image uses GCC 13.3 and GNU ld 2.42. The image is linked with --emit-relocs and -pie, using the x86 linker script which collects the runtime relocations in .rel.dyn. Despite combreloc being the default, the resulting .rel.dyn is NOT ordered by relocation address in this configuration. Explicitly adding -z combreloc does NOT change it. The failing image has 15,610 entries, with 449 cases where an in-image relocation address is lower than the preceding one. The existing monotonic check skips each such entry. This leaves pointers unrelocated, and both qemu-x86 and coreboot subsequently fail during relocated init with: initcall_run_r(): initcall log_init() failed ### ERROR ### Please RESET the board ### >> >> Check that each relocation target lies within the image instead. Apply >> the same correction to the 32-bit and 64-bit paths. >> >> Fixes: b50b1633c022 ("x86: Add support for 64-bit relocation") >> Fixes: 62f7970a5a06 ("x86: Add error checking to x86 relocation code") >> Signed-off-by: Jerome Forissier <[email protected]> >> >> arch/x86/lib/relocate.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) > >> diff --git a/arch/x86/lib/relocate.c b/arch/x86/lib/relocate.c >> @@ -117,7 +115,7 @@ static void do_elf_reloc_fixups32(unsigned int >> text_base, uintptr_t size, >> /* Check that the location of the relocation is in .text */ >> if (offset_ptr_rom >= (Elf32_Addr *)(uintptr_t)text_base && >> - offset_ptr_rom > last_offset) { >> + offset_ptr_rom < (Elf32_Addr *)(text_base + size)) { > > One thing I'd like to check.. the ordering check came from commit > 62f7970a, which says it is there to catch corrupted relocation data. > On x86 the .bss is an OVERLAY on __rel_dyn_start in both u-boot.lds > and u-boot-64.lds, so a stray BSS write before relocation overwrites > entries. The 'must increase' test was a cheap way to skip that > garbage. With this patch, a corrupted entry that lands inside the > image is applied. In the 32-bit case that means a '+=' on whatever is > there, as long as the value passes the target check. I agree, the bounds check is weaker than the ordering check for detecting corruption, but the ordering check was based on a wrong assumption. I don't see a reliable way to distinguish an in-range corrupted entry from a valid, unordered entry. > I'm fine with the change, since the upper bound covers most of this, > but please can you mention it in the commit message, so it is clear > that the bounds check now does this job. The comment above > do_elf_reloc_fixups() points readers at that commit, so the two should > stay consistent. Sure. I'll send a v2. Thanks, -- Jerome > > Regards, > Simon
