Page MenuHomeFreeBSD

stand: Improve error handling when loading ELF files
AcceptedPublic

Authored by jhb on Wed, Jul 29, 7:23 PM.

Details

Summary

Previously all the 'goto out' statements after the image was loaded into
memory returned success rather than an error. This is despite comments
indicating some of these conditions were in fact errors, and some of
these error conditions (such as missing PT_DYNAMIC) are treated as errors
in the kernel linker.

In addition, when failing to looking up the symbols for the linker
set, those cases returned failure leaking memory (though it's clear
from the original code from commit ca49b3342d1e that only the second
failure was intended to be an actual error).

To avoid more confusion, move the assignment of ret to just before
the out label so that goto out always returns an error. This is a
more consistent pattern with other code in the tree that tends to use
labels for the error case.

Restructure some other code to avoid a few bogus errors.
Specifically, a symbol table is not required so don't treat lack of a
symbol table as an error. Also, if the start symbol for the module
metadata linker set is not found, don't treat that as an error either.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75281
Build 72164: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Wed, Jul 29, 7:23 PM
stand/common/load_elf.c
854

The comment kind of implies this should be an error, but with most of these I took the view that it can be permissible to have a foo.ko that doesn't contain any module_t's. For example, you could have a module that only has sysctls (I have written such modules in the past). So for example, not having the __start_set_modmetdata_set symbol shouldn't be an error (though having the start but not the end is certainly an error!). Some of these earlier checks though I'm less sure about. Do we really need a symbol table and PT_DYNAMIC?

Re-reading sys/kern/link_elf.c, it seems that it does require PT_DYNAMIC to be present and valid, but it doesn't require a valid symbol table, so I might amend this to treat a few more of these as fatal.

jhb retitled this revision from stand: Improve error handling when parsing ELF module metadata to stand: Improve error handling when loading ELF files.Wed, Jul 29, 8:26 PM
jhb edited the summary of this revision. (Show Details)
jhb edited the summary of this revision. (Show Details)

Better align with kernel linker

My testing was to boot a kernel with a ccr_load="YES" in /boot/loader.conf (ccr.ko depends on if_cxgbe.ko) and verify it still parsed the metadata correctly and auto-loaded the dependency in the loader. That and it also didn't choke on a valid kernel but still booted ok. I don't have any degenerate kernel modules lying around to test further (e.g. a kernel module with only a sysinit).

This revision is now accepted and ready to land.Thu, Jul 30, 9:17 AM