Differential D20218

Do not return NULL from the Intel hw PRNG ifunc resolver.
ClosedPublic
Actions

Authored by markj on May 9 2019, 8:44 PM.

Details

Reviewers

cem
kib
delphij
jmg

Group Reviewers

secteam
O3: Kernel Random Numbers Generator	(Owns No Changed Paths)

Commits

rS347423: Avoid returning a NULL pointer from the Intel hw PRNG ifunc resolver.

Summary

The kernel linker and its users don't handle a symbol value of 0, and
the GNU ifunc documentation suggests that NULL is not a valid return
value from a resolver. ("The implementation functions’ declarations
must match the API of the function being implemented.")

I considered asserting against such a return value in
elf_lookup_ifunc(), but it would simply cause a reset before the console
is initialized and thus be a pain to debug. Maybe it's worth doing
anyway.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24177
Build 23024: arc lint + arc unit

Event Timeline

markj created this revision.May 9 2019, 8:44 PM

Harbormaster completed remote builds in B24176: Diff 57228.May 9 2019, 8:44 PM

markj added reviewers: cem, kib.May 9 2019, 8:44 PM

Huh, I figured linker failure would prevent MOD_LOAD and thus it wouldn't matter. I had thought about providing a 3rd panic function like this but didn't know if it was required and couldn't think of a reason it might be. Guess I was mistaken :-).

sys/dev/random/ivy.c
104	it's more like we shouldn't be invoking x86_rng_store at all if neither rdrand nor rdseed was detected -- MOD_LOAD() shouldn't register our callback unless HW support is present. So I guess I'm saying, this should panic because of an invariants violation, not "missing hardware support".

This revision is now accepted and ready to land.May 9 2019, 8:50 PM

In D20218#435593, @cem wrote:

Huh, I figured linker failure would prevent MOD_LOAD and thus it wouldn't matter. I had thought about providing a 3rd panic function like this but didn't know if it was required and couldn't think of a reason it might be. Guess I was mistaken :-).

Kernel ifuncs are resolved quite early during boot, well before MOD_LOAD. And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

Use a better panic message and switch to KASSERT().

This revision now requires review to proceed.May 9 2019, 8:59 PM

Harbormaster completed remote builds in B24177: Diff 57229.May 9 2019, 8:59 PM

In D20218#435600, @markj wrote:

Kernel ifuncs are resolved quite early during boot, well before MOD_LOAD.

Right, I expected ifunc resolution long before MOD_LOAD -- it's where has_rdrand etc are initialized for a reason. But I figured the later mod_load was the only possible way for the 0 symbol to be accessed, so as long as it was prevented there. Anyway, dtrace.

And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

Sure, I guess it's really dtrace that doesn't like zero symbols. We could monkey patch this or we could change dtrace to just ignore ifuncs that resolve to zero. I don't know if there are other consumers than dtrace that might stumble over the same obstacle, though it seems like maybe a pain point for further adoption of ifuncs.

sys/dev/random/ivy.c
104	Hm, I preferred panic -- KASSERT is compiled out on !INVARIANTS and this is always wrong. If you prefer KASSERT, we should at least return failure (0) instead of no return value, to match function signature.

This is the trail of the dtrace fbt panic ?

sys/dev/random/ivy.c
105	I am surprised that this function compiles. You should return something from it. I vote to panic() always.

Go back to panic().

Harbormaster completed remote builds in B24179: Diff 57232.May 9 2019, 10:04 PM

markj added inline comments.May 9 2019, 10:04 PM

sys/dev/random/ivy.c
104	Ok. KASSERT seemed reasonable since the function should be unreachable unless there's a bug somewhere.
105	It compiles with INVARIANTS on because the branch in the KASSERT() is always taken.

cem accepted this revision.May 9 2019, 10:07 PM

This revision is now accepted and ready to land.May 9 2019, 10:07 PM

In D20218#435623, @cem wrote:

In D20218#435600, @markj wrote:

And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

Sure, I guess it's really dtrace that doesn't like zero symbols. We could monkey patch this or we could change dtrace to just ignore ifuncs that resolve to zero. I don't know if there are other consumers than dtrace that might stumble over the same obstacle, though it seems like maybe a pain point for further adoption of ifuncs.

I think it's reasonable to assume that a function symbol with non-zero size has a sane value. I was thinking of a way to enforce a non-NULL return value at compile-time, akin to the nonnull attribute, but I don't see a good way to do it.

kib accepted this revision.May 9 2019, 10:21 PM

kib added inline comments.