Page MenuHomeFreeBSD

Make more use of arc4random() in the kernel.
Needs RevisionPublic

Authored by pfg on May 18 2016, 8:21 PM.

Details

Summary

This is an attempt to use arc4random() more extensively in the kernel.
Particularly interesting may be ipfw and congestion control, although
the uses there seem to be little related with security.

I excluded using arc4random in contrib'ed code, in tests, in devices and in the screensavers. devices may have to be investigated later.

Test Plan

I rebuilt my kernel and I am running it.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3816
Build 3859: arc lint + arc unit

Event Timeline

pfg updated this revision to Diff 16544.May 18 2016, 8:21 PM
pfg retitled this revision from to Make more use of arc4random() in the kernel..
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)

Hello;

The improvement is probably not huge but given that we can provide better randomness ... why not? :)

This touches a bunch of code in the kernel and I want to make sure the code was not side-stepped in the past from conversion for some reason. Thanks for reviewing.

pfg updated this object.May 18 2016, 8:49 PM
pfg edited the test plan for this revision. (Show Details)
lstewart edited edge metadata.May 18 2016, 11:47 PM
In D6442#136542, @pfg wrote:

Hello;
The improvement is probably not huge but given that we can provide better randomness ... why not? :)

Without numbers quantifying the impact of switching to arc4random(), it is difficult to agree this is a good idea or provide any useful feedback. I don't possess any intuitive notion of the overhead vs entropy improvement compared with random().

pfg added a comment.May 19 2016, 3:02 AM
In D6442#136542, @pfg wrote:

Hello;
The improvement is probably not huge but given that we can provide better randomness ... why not? :)

Without numbers quantifying the impact of switching to arc4random(), it is difficult to agree this is a good idea or provide any useful feedback. I don't possess any intuitive notion of the overhead vs entropy improvement compared with random().

From my measurements in filesystems, the performance difference was not quantifiable. We just happen to use random numbers very rarely in filesystems and it basically happens during formatting.
Also take into account that:

  • The regular random() is rather slow.
  • Modern processors (which I don't have access to) should take stuff like arc4random() rather easily.

It very much depends on the application and the type of load so I am not suggesting just committing this patch. It has to be evaluated by different developers and it's unlikely we could do this for, say, 11-Release. Perhaps we could incorporate it early in the 12-current cycle though which would give it ample testing for a year or two.

For now I would appreciate reviews of the application: there was some scaling that had to be adjusted for the congestion algorithms. Are those OK?

delphij requested changes to this revision.May 20 2016, 6:27 PM
delphij edited edge metadata.

I think we should get Chacha20, getentropy(), etc. first.

Please do not use arc4random() % but instead use arc4random_uniform (we should probably get ChaCha20 in base first) by the way.

sys/net/if_spppsubr.c
4346

I don't think this code is right. Why can't the whole challenge be generated by e.g. arc4random_buf (not in kernel but we should have it)?

This revision now requires changes to proceed.May 20 2016, 6:27 PM
pfg added a comment.May 20 2016, 6:43 PM

I think we should get Chacha20, getentropy(), etc. first.

I think that is a PR with a patch for that, but an important part
of the patch touches the VM. That needs a review from the experts.

Please do not use arc4random() % but instead use arc4random_uniform (we should probably get ChaCha20 in base first) by the way.

We don't have arc4random_uniform in the kernel.

I am considering dropping completely this patch and instead rewrite random() to use a better and faster, but not crypto-safe, algorithm based on KISS.

emaste added a subscriber: emaste.Mar 21 2017, 4:50 PM