linker: Harden checks for ELFs

[code_fedang.net]

Summary

Added checks that abort the load of ET_DYN modules when the
program segments are overlapping or unsorted.
Also validate that their size is valid and not overflowing.

Event: EuroBSDCon 2026

Test Plan

kyua report -r /root/.kyua/store/results.usr_obj_usr_src_amd64.amd64_tests_sys_kld_checkdir_usr_tests_sys_kld.20260922-135000-264526.db

> Summary

Results read from /root/.kyua/store/results.usr_obj_usr_src_amd64.amd64_tests_sys_kld_checkdir_usr_tests_sys_kld.20260922-135000-264526.db
Test cases: 16 total, 0 skipped, 0 expected failures, 0 broken, 0 failed
Total time: 0.882s

Before the changes it just crashed during testing

[code_fedang.net]

Original bugzilla report:217432

[bnovkov]

sys/kern/link_elf.c

1121–1123

I think we should enforce this assumption (i.e., return EINVAL or something similar) instead of sorting the segments.

Thinking about this from a user's standpoint, a kldload triggering this assertion almost certainly means that something went wrong during linking or that somebody is trying to mess with the kernel.
It would make more sense to have the operation fail and let the user know that something's up instead of silently correcting the error.

[code_fedang.net]

sys/kern/link_elf.c

1121–1123

Reading through the ELF specification this is not actually a requirement.
Putting them in order is the de facto standard, but I can see some obscure compiler doing whatever they want.
What would the error message say since the file is technically in-spec?

[bnovkov]

sys/kern/link_elf.c

1121–1123

An ENOEXEC with something along the lines of "Improperly sorted segments " is fine.

[code_fedang.net]

Check that segments are already sorted

[code_fedang.net]

Fix other panics found during testing

[code_fedang.net]

Added test cases

[code_fedang.net]

Clean up

[bnovkov]

Generally LGTM, two nits aside. I'll test this a bit more in the coming days and land it if nothing obvious pops up.

tests/sys/kld/link_elf.c

2

Missing copyright header.

163

Please move this definition below the test case definitions.

[code_fedang.net]

Add copyright text

[code_fedang.net]

done

[bnovkov]

Could you please rebase the patch to a recent main? I can't cleanly apply the patch for testing.

[code_fedang.net]

Rebase against main. Apparently some of these fixes were pushed already

[code_fedang.net]

since the code that caused the panic has already been fixed on main, maybe we should rename this commit to "fix edgecases" or similar?

[bnovkov]

In D59710#1385442, @code_fedang.net wrote:

since the code that caused the panic has already been fixed on main, maybe we should rename this commit to "fix edgecases" or similar?

Yes please, something along the lines of "harden ELF checks" would be great.

tests/sys/kld/link_elf.c

2

Please use the shorter copyright header as noted in style(9).

[bnovkov]

CCing @jhb and @jrtc27 since they've reviewed a similar patch (D58542) recently.

[code_fedang.net]

Shorten copyright notice

[jrtc27]

Alternatively, why not just adjust the existing loop that populates segs to instead take min(p_vaddr) and max(p_vaddr + p_memsz)? It wouldn't be hard to do and would remove the need for this restriction (and without any complex sorting). The only thing you'd miss is handling overlapping segments, but to be honest I don't think that would necessarily be a problem, we'd do exactly what the file asked for and load the segments in that order, but it just might result in the module shooting itself in the foot, which I don't see as any different from the countless other ways a module can corrupt its own internal state and panic your kernel. At the end of the day if you're loading a module into the kernel you're trusting the module to not itself be a panic-fest, all the kernel linker should be doing is avoiding confusing/corrupting things *outside* the module.

[code_fedang.net]

In D59710#1385456, @jrtc27 wrote:

Alternatively, why not just adjust the existing loop that populates segs to instead take min(p_vaddr) and max(p_vaddr + p_memsz)? It wouldn't be hard to do and would remove the need for this restriction (and without any complex sorting). The only thing you'd miss is handling overlapping segments, but to be honest I don't think that would necessarily be a problem, we'd do exactly what the file asked for and load the segments in that order, but it just might result in the module shooting itself in the foot, which I don't see as any different from the countless other ways a module can corrupt its own internal state and panic your kernel. At the end of the day if you're loading a module into the kernel you're trusting the module to not itself be a panic-fest, all the kernel linker should be doing is avoiding confusing/corrupting things *outside* the module.

I thought about that at the start, but ended up with the extra check since the subsequent code seems to rely on that.
Overall, I think a sane policy for malformed modules would be: if it has to crash, it should do so outside of the linker.

[jrtc27]

In D59710#1386475, @code_fedang.net wrote:

In D59710#1385456, @jrtc27 wrote:

Alternatively, why not just adjust the existing loop that populates segs to instead take min(p_vaddr) and max(p_vaddr + p_memsz)? It wouldn't be hard to do and would remove the need for this restriction (and without any complex sorting). The only thing you'd miss is handling overlapping segments, but to be honest I don't think that would necessarily be a problem, we'd do exactly what the file asked for and load the segments in that order, but it just might result in the module shooting itself in the foot, which I don't see as any different from the countless other ways a module can corrupt its own internal state and panic your kernel. At the end of the day if you're loading a module into the kernel you're trusting the module to not itself be a panic-fest, all the kernel linker should be doing is avoiding confusing/corrupting things *outside* the module.

I thought about that at the start, but ended up with the extra check since the subsequent code seems to rely on that.
Overall, I think a sane policy for malformed modules would be: if it has to crash, it should do so outside of the linker.

My point is, beyond the lazy way to compute min(p_vaddr) and max(p_vaddr + p_memsz), I don't think the subsequent code does rely on that, so why not just make it less lazy? It's fewer lines of code than verifying the assumptions hold.