Details

Reviewers

kib
markj
manu
jmg

Group Reviewers

csprng
O3: Kernel Random Numbers Generator

Commits

rS366622: random(4) FenestrasX: Push root seed version to arc4random(3)

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

kib added inline comments.Jan 5 2020, 2:55 AM

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.

cem added inline comments.Jan 5 2020, 3:20 AM

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`

kib added inline comments.Jan 5 2020, 5:26 AM

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.

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

Harbormaster completed remote builds in B28490: Diff 66370.Jan 5 2020, 7:00 AM

cem added inline comments.Jan 5 2020, 7:00 AM

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.

kib added inline comments.Jan 5 2020, 7:29 AM

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.

Harbormaster completed remote builds in B28565: Diff 66542.Jan 9 2020, 9:59 AM

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.

Harbormaster completed remote builds in B28921: Diff 67307.Jan 26 2020, 7:07 AM

cem added inline comments.Jan 26 2020, 7:07 AM

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
131 ↗	(On Diff #67307)	There is no check for version there.
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).

Update to latest draft.

Harbormaster completed remote builds in B34044: Diff 77986.Oct 7 2020, 7:07 PM

cem added inline comments.Oct 7 2020, 7:09 PM

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.

kib added inline comments.Oct 8 2020, 12:49 AM

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.

cem added inline comments.Oct 8 2020, 12:58 AM

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.

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

Harbormaster completed remote builds in B34063: Diff 78018.Oct 8 2020, 5:23 PM

kib added inline comments.Oct 8 2020, 5:56 PM

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.

cem added inline comments.Oct 8 2020, 6:23 PM

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).

Add to LA57 amd64 ABI.

Harbormaster completed remote builds in B34064: Diff 78020.Oct 8 2020, 6:24 PM

cem added inline comments.Oct 8 2020, 6:25 PM

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

kib added inline comments.Oct 9 2020, 1:01 AM

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.

Simplify disable mechanism
Drop AT_ define to next contiguous index

Harbormaster completed remote builds in B34081: Diff 78074.Oct 10 2020, 12:40 AM

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

Harbormaster completed remote builds in B34082: Diff 78075.Oct 10 2020, 12:56 AM

kib accepted this revision.Oct 10 2020, 1:45 PM

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.Oct 10 2020, 1:45 PM

cem added inline comments.Oct 10 2020, 5:27 PM

sys/kern/kern_sharedpage.c
358 ↗	(On Diff #78075)	No, I guess it doesn't. Will fix.

Drop ifdef from exec_sysvec_init_secondary() fxrng assertion, base ptr initialization

This revision now requires review to proceed.Oct 10 2020, 5:31 PM

Harbormaster completed remote builds in B34089: Diff 78085.Oct 10 2020, 5:31 PM

This revision was not accepted when it landed; it landed in state Needs Review.Oct 10 2020, 9:52 PM

Closed by commit rS366622: random(4) FenestrasX: Push root seed version to arc4random(3) (authored by cem). · Explain Why

This revision was automatically updated to reflect the committed changes.

cem added a commit: rS366622: random(4) FenestrasX: Push root seed version to arc4random(3).

Owners added a reviewer: O3: Kernel Random Numbers Generator.Oct 10 2020, 9:52 PM

Herald added a reviewer: jmg. · View Herald TranscriptOct 10 2020, 9:52 PM

Herald added a subscriber: imp. · View Herald Transcript

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?

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 78092

head/lib/libc/gen/arc4random.h

head/lib/libc/gen/arc4random.c

head/lib/libc/gen/auxv.c

head/sys/amd64/amd64/elf_machdep.c

head/sys/arm/arm/elf_machdep.c

head/sys/arm64/arm64/elf32_machdep.c

head/sys/arm64/arm64/elf_machdep.c

head/sys/compat/ia32/ia32_sysvec.c

head/sys/dev/random/fenestrasX/fx_brng.c

head/sys/dev/random/fenestrasX/fx_main.c

head/sys/i386/i386/elf_machdep.c

head/sys/kern/imgact_elf.c

head/sys/kern/kern_sharedpage.c

head/sys/mips/mips/elf_machdep.c

head/sys/mips/mips/freebsd32_machdep.c

head/sys/powerpc/powerpc/elf32_machdep.c

head/sys/powerpc/powerpc/elf64_machdep.c

head/sys/riscv/riscv/elf_machdep.c

head/sys/sys/elf_common.h

head/sys/sys/sysent.h

head/sys/sys/vdso.h

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 78092

head/lib/libc/gen/arc4random.h

head/lib/libc/gen/arc4random.c

head/lib/libc/gen/auxv.c

head/sys/amd64/amd64/elf_machdep.c

head/sys/arm/arm/elf_machdep.c

head/sys/arm64/arm64/elf32_machdep.c

head/sys/arm64/arm64/elf_machdep.c

head/sys/compat/ia32/ia32_sysvec.c

head/sys/dev/random/fenestrasX/fx_brng.c

head/sys/dev/random/fenestrasX/fx_main.c

head/sys/i386/i386/elf_machdep.c

head/sys/kern/imgact_elf.c

head/sys/kern/kern_sharedpage.c

head/sys/mips/mips/elf_machdep.c

head/sys/mips/mips/freebsd32_machdep.c

head/sys/powerpc/powerpc/elf32_machdep.c

head/sys/powerpc/powerpc/elf64_machdep.c

head/sys/riscv/riscv/elf_machdep.c

head/sys/sys/elf_common.h

head/sys/sys/sysent.h

head/sys/sys/vdso.h

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

Revision Contents
Changeset List