Page MenuHomeFreeBSD

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

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.May 9 2019, 8:44 PM
markj added reviewers: cem, kib.May 9 2019, 8:44 PM
cem accepted this revision.May 9 2019, 8:50 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 ↗(On Diff #57228)

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
markj marked an inline comment as done.May 9 2019, 8:59 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.

markj updated this revision to Diff 57229.May 9 2019, 8:59 PM

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

This revision now requires review to proceed.May 9 2019, 8:59 PM
cem added a comment.May 9 2019, 9:23 PM

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 ↗(On Diff #57229)

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.

kib added a comment.May 9 2019, 9:26 PM

This is the trail of the dtrace fbt panic ?

sys/dev/random/ivy.c
105 ↗(On Diff #57229)

I am surprised that this function compiles. You should return something from it.

I vote to panic() always.

markj updated this revision to Diff 57232.May 9 2019, 10:04 PM
markj marked an inline comment as done.

Go back to panic().

markj added inline comments.May 9 2019, 10:04 PM
sys/dev/random/ivy.c
104 ↗(On Diff #57229)

Ok. KASSERT seemed reasonable since the function should be unreachable unless there's a bug somewhere.

105 ↗(On Diff #57229)

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
markj added a comment.May 9 2019, 10:16 PM
In D20218#435623, @cem 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.
sys/dev/random/ivy.c
117 ↗(On Diff #57232)

In fact you could return x86_rdseed_store if !has_rdrand, hardware then would trap if RDSEED is not implemented.

markj added inline comments.May 9 2019, 10:24 PM
sys/dev/random/ivy.c
117 ↗(On Diff #57232)

Or even always return x86_rdrand() when !has_rdseed. I think I prefer having an explicit panic() though.

delphij accepted this revision.May 10 2019, 12:15 AM
This revision was automatically updated to reflect the committed changes.