limine-bootloader / limine

Modern, advanced, portable, multiprotocol bootloader and boot manager.
https://limine-bootloader.org
BSD 2-Clause "Simplified" License
1.87k stars 141 forks source link

protos/multiboot2: fix issues with booting relocatable multiboot2 payloads #411

Closed monkuous closed 2 months ago

monkuous commented 2 months ago

This PR fixes two issues that caused booting relocatable multiboot2 payloads to fail:

  1. The order of the parameters to the ALIGN_* macros was incorrect (so alignment was aligned to min/max addr instead of the other way around). This caused incorrect alignment, or a division exception if lowest-to-highest allocation is preferred and the minimum address is zero.
  2. reloc_slide was calculated as the absolute offset between the declared base address and the actual base address, but used as if it weren't absolute. This caused the relocated entry point address to be calculated incorrectly if highest-to-lowest allocation is preferred.
mintsuki commented 2 months ago

Hi, thanks for the PR!

The first commit LGTM, but the second one would probably be best if changed to use a signed int64_t instead of a uint64_t for reloc_slide (and casting relocated_base to int64_t as well in the assignment).

monkuous commented 2 months ago

Thanks for the feedback. I opted not to use int64_t in this case because both its calculation and usage relies on integer overflow, which is undefined behavior for signed integers.

mintsuki commented 2 months ago

I'm not sure what's the signed overflow? From a quick look it only seems to cross the negative<->positive barrier which is perfectly defined behaviour for signed integers (and not considered overflow). I may have missed something though.

monkuous commented 2 months ago

Nevermind, now that I'm looking at it again, you're right. I'll change it to be signed.

mintsuki commented 2 months ago

LGTM thanks!