Page MenuHomeFreeBSD

random(4) fenestrasX: Push root seed version to arc4random(3)
ClosedPublic

Authored by cem on Dec 16 2019, 7:34 PM.

Details

Summary

Push the root seed version to userspace through the VDSO page.

arc4random(3) obtains a pointer to the root seed version published by
the kernel in the shared page at allocation time. Like arc4random(9),
it maintains its own per-process copy of the seed version corresponding
to the root seed version at the time it last rekeyed. On read requests,
the process seed version is compared with the version published in the
shared page; if they do not match, arc4random(3) reseeds from the
kernel.

This change does not implement the fenestrasX concept of PCPU userspace
generators seeded from a per-process base generator. That change is
left for future discussion/work.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
lib/libc/gen/arc4random.c
138 ↗(On Diff #66358)

libraries should not kill the process.

lib/libc/gen/arc4random.h
103 ↗(On Diff #66358)

Please do not emit messages from libc.

126 ↗(On Diff #66358)

This puns on int/long for ILP32. Since you use #ifdef __LP64 already, I think this should be type-correct as well.

sys/kern/kern_sharedpage.c
299 ↗(On Diff #66358)

Why do you need this persistent 'zero' structure with single use ? You can copy from zero_region and save some memory.

337 ↗(On Diff #66358)

Please move the declaration to the start of the function.

sys/sys/vdso.h
66 ↗(On Diff #66358)

Why not use 32bit generation on 64bit arch ?

97 ↗(On Diff #66358)

The prototype definitely does not need the #ifdef around.

lib/libc/gen/arc4random.c
138 ↗(On Diff #66358)

Right. This assertion should never fire.

Note that this library is already special in that _rs_init() abort()s if _rs_allocate() fails. This is not new in this revision.

lib/libc/gen/arc4random.h
103 ↗(On Diff #66358)

Ok

126 ↗(On Diff #66358)

How does it pun on int/long? fxrng_root_generationp is unsigned long*

sys/kern/kern_sharedpage.c
299 ↗(On Diff #66358)

Sure, that would be fine.

337 ↗(On Diff #66358)

Ok

sys/sys/vdso.h
66 ↗(On Diff #66358)

Why use 32bit generation on 64bit arch? The 64 bit counter is canonical and cheap on 64bit, might as well stick with the longer generation there.

97 ↗(On Diff #66358)

Hm, I'm not sure why not. It exists only conditional on option RANDOM_FENESTRASX

lib/libc/gen/arc4random.h
126 ↗(On Diff #66358)

Hm, yes, you do type-punning with fxrng_root_generation instead. One more reason to use 32bit IMO.

sys/sys/vdso.h
66 ↗(On Diff #66358)

I am not convinced that 64bit are useful if 32bit are enough.

But regardless, I do not think that fx_generation32 is needed if you provide fx_generation. 32bit read can be done from the proper word of fx_generation (high word on BE).

97 ↗(On Diff #66358)

But prototype is valid regardless. Also it puts a dependency on the opt_ header before this one.

cem marked 6 inline comments as done.
  • Remove warn from libc
  • Use more verbose 32/64 atomic spelling instead of long
  • Use zero_region
  • C89-ify a scoped variable declaration to top of function
sys/sys/vdso.h
66 ↗(On Diff #66358)

I thought we already discussed that reading 32 bits out of kernel 64 bit atomic write wasn't safe. I'm confused why you propose it now.

The generation32 costs nothing; we've already allocated a full cache line. We have 52 bytes to spare.

I will leave it as is.

97 ↗(On Diff #66358)

The prototype is not valid if the option isn't present; that's the point.

If the option isn't present, using that declaration will eventually result in a link error. It's better to cause a compile error earlier on than a link error at the end of the kernel build, IMO.

Opt header is not a concern; opt_global.h already precedes all headers.

sys/sys/vdso.h
66 ↗(On Diff #66358)

We discussed that reading both high and low words is not safe because we can see inconsistent results without some form of lock. But reading just LSW is fine, even if the write is not 64bit atomic.

I have one more suggestion for this stuff. Put a version into the struct vdso_fxrng_generation, and make the current version number one. This would allow to expand it later without allocating another AT_ auxv id, and even more important, it allows to implement global disable knob by zeroing out version. I found a way to disable vdso timehands quite life-saving when users reported some issues with it (implemented as a sysctl knob).

Version the VDSO structure.

There should be a knob which switches the version to 0, disabling the feature at runtime.
I stil think that having both 64 and 32bit data word with identical content is ugly and even erroneous ABI decision.

lib/libc/gen/arc4random.h
90 ↗(On Diff #66542)

Why use _1 and not _CURR ?

Add sysctl knob to disable VDSO page ABI.

lib/libc/gen/arc4random.h
90 ↗(On Diff #66542)

Why _CURR? This version of the code understands version 1 of the ABI; if CURR is bumped, this code should still be associated with version 1, not whatever moving target CURR is.

I still do not understand the need for both fx_generation and fx_generation32, and why fx_generation is 64bit.

lib/libc/gen/arc4random.c
127 ↗(On Diff #67307)

Again, this seems to miss the version check.

lib/libc/gen/arc4random.h
90 ↗(On Diff #66542)

This is the common practice, look e.g. at the sys/conf.h. The motivation is that ver 2 typically differs from ver 1 by some adjustments, instead of being rewritten from scratch. Correspondingly, the code gets some fiddling with, which makes one less place to change (and forget).

131 ↗(On Diff #67307)

There is no check for version there.

cem marked an inline comment as done.

Update to latest draft.

lib/libc/gen/arc4random.c
127 ↗(On Diff #67307)

Why check the version repeatedly? Most other VDSO knobs of this kind are checked only at process startup; I don't see any reason to pessimize this use in particular.

Like the other patches in this series, I would like to land this soon. Like the other patches, it should not have any functional change on systems without a RANDOM_FENESTRASX kernel. I've addressed all of kib's earlier objections that I understood.

lib/libc/gen/arc4random.c
127 ↗(On Diff #67307)

Other vdso data blobs (timehands) allow to dynamically disable its use. The only mechanism your vdso_fxrngp provides for this is version check.

sys/kern/kern_sharedpage.c
371 ↗(On Diff #77986)

This function needs update to init sv2->sv_handle fxrng_gen_base.

lib/libc/gen/arc4random.c
127 ↗(On Diff #67307)

This can be done by just halting update publishing on the VDSO page. sysctl_fxrng_vdso and fxrng_push_seed_generation do not do that yet, but could pretty easily.

sys/kern/kern_sharedpage.c
371 ↗(On Diff #77986)

Thanks, will investigate.

cem marked 2 inline comments as done.
  • Initialize fxrng offset in secondary sysvecs
  • Halt seed version update while/if shared page feature is disabled, which has the effect of disabling the feature for existing processes
sys/amd64/amd64/elf_machdep.c
110 ↗(On Diff #78018)

And what about la57 ABI ?

sys/kern/kern_sharedpage.c
315 ↗(On Diff #78018)

This is very strange 'disable'. It only makes newly exec-ed processes to ignore the fxrng_shpage, and if process saw _DISABLED then it is forever (until exec or exit).

I really do not see this 'disable' as useful, it provides very non-obvious behavior. Timecounter provides the 'disable' switch as an emergency measure which appeared useful more than once in field debugging and making people machines operational. Main point of it was that all instances of libc reacted to it right away.

376 ↗(On Diff #78018)

Assert should be added that secondary sysvec has the same value of SV_RNG_SEED_VER flag as the primary.

sys/sys/elf_common.h
970 ↗(On Diff #78018)

Why is it 99 ? You are blowing several arrays without a reason.

sys/amd64/amd64/elf_machdep.c
110 ↗(On Diff #78018)

It is newer than this patch. Will add.

sys/kern/kern_sharedpage.c
315 ↗(On Diff #78018)

I really don't know what you want in a disable mechanism. I don't see how it can possibly be useful for this feature. Just as a reminder, RANDOM_FENESTRASX will land completely compiled-out of the GENERIC kernel anyway. I've tried to add a disable mechanism because you want it, but now you are complaining that it can't be re-enabled for processes that saw it was disabled.

I don't understand the concern on either end. I don't think this needs a disable mechanism, nor do I think it is especially significant how quickly the feature is re-enabled if the user already turned it off once.

I've been running this code in kernel and libc on my workstation and test VMs for something like two years now without issue.

sys/sys/elf_common.h
970 ↗(On Diff #78018)

This can be dropped to 33 before commit. It worked better for a WIP patch, when there were repeated collisions with other new auxv values taking the final slot (all of BSDFLAGS through PS_STRINGS collided with this change at one point in time).

cem marked an inline comment as done.

Add to LA57 amd64 ABI.

sys/kern/kern_sharedpage.c
315 ↗(On Diff #78018)

One year*

sys/kern/kern_sharedpage.c
315 ↗(On Diff #78018)

Normally disable in vdso causes all currently running FreeBSD-ABI processes to stop using corresponding vdso facility. In reverse, enable causes them to start using it at the moment of enablement. This is not true for your variant of disable, which only acts once for the program, at exec time.

I am sure that if somebody sees potential issues with the patch the issue is fixed in advance, the 'disable' knob is for emergency and debug. It helped me to save several user machines for timecounter.

Anyway, I do not want to block the feature on that disagreement. Please fix AT_FXRNG numeric value and go ahead.

sys/sys/elf_common.h
970 ↗(On Diff #78020)

You still did not changed the number.

cem marked 2 inline comments as done.
  • Simplify disable mechanism
  • Drop AT_ define to next contiguous index
cem marked an inline comment as done.

Add missing assertion that secondary sysvec flags matches primary for FXRNG.

kib added inline comments.
sys/kern/kern_sharedpage.c
358 ↗(On Diff #78075)

Does this assert and assignment to base below need #ifdef braces ? If support is not compiled in, base should be NULL. Flag should be consistent anyway.

This revision is now accepted and ready to land.Sat, Oct 10, 1:45 PM
sys/kern/kern_sharedpage.c
358 ↗(On Diff #78075)

No, I guess it doesn't. Will fix.

cem marked an inline comment as done.
  • Drop ifdef from exec_sysvec_init_secondary() fxrng assertion, base ptr initialization
This revision now requires review to proceed.Sat, Oct 10, 5:31 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sat, Oct 10, 9:52 PM
This revision was automatically updated to reflect the committed changes.

This broke building on macOS+Linux (https://github.com/freebsd/freebsd/runs/1236550352) due to

fatal error: sys/vdso.h: No such file or directory

Could you please check whether D26738 is an appropriate way of fixing this?