Differential D54259

armv8rng: Fix an inverted test in random_rndr_read_one()
ClosedPublic
Actions

Authored by markj on Tue, Dec 16, 7:56 PM.

Details

Reviewers

andrew

Group Reviewers

arm64

Commits

rG93811883500b: armv8rng: Fix an inverted test in random_rndr_read_one()

Summary

If we get a random number, the NZCV is set to 0b0000. Then
"cset %w1, ne" will test whether Z == 0 and set %w1 to 1 if so.
More specifically, "cset %w1, ne" maps to "csinc %w1, wzr, wzr, eq",
which stores 0 in %w1 when NZCV == 0b0100 and 1 otherwise.

Thus, on a successful read we expect ret != 0, so the loop condition
needs to be fixed.

Fixes: 9eecef052155 ("Add an Armv8 rndr random number provider")
Reported by: Kevin Day <kevin@your.org>

Test Plan

I have no hardware to test this with.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 69316
Build 66199: arc lint + arc unit

Event Timeline

markj created this revision.Tue, Dec 16, 7:56 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, Dec 16, 7:56 PM

markj requested review of this revision.Tue, Dec 16, 7:56 PM

Harbormaster completed remote builds in B69316: Diff 168219.Tue, Dec 16, 7:56 PM

Herald added a subscriber: obrien. · View Herald TranscriptTue, Dec 16, 7:56 PM

markj edited the test plan for this revision. (Show Details)Tue, Dec 16, 7:56 PM

So the current code tries 10 times to hit a failure, and will use the last value if all 10 reads succeeded, given the condition after the loop is correct? Importantly, there's no security issue here, because we didn't actually report a successful read when it failed?

In D54259#1239893, @jrtc27 wrote:

So the current code tries 10 times to hit a failure, and will use the last value if all 10 reads succeeded, given the condition after the loop is correct? Importantly, there's no security issue here, because we didn't actually report a successful read when it failed?

Yes, I think that's right. At worst we are not getting entropy because reading the register 10 times back-to-back causes a failure. Our default CSPRNG by design draws entropy from many different sources, so this isn't catastrophic.

andrew accepted this revision.Thu, Dec 18, 12:59 PM

This revision is now accepted and ready to land.Thu, Dec 18, 12:59 PM

Closed by commit rG93811883500b: armv8rng: Fix an inverted test in random_rndr_read_one() (authored by markj). · Explain WhyThu, Dec 18, 7:47 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG93811883500b: armv8rng: Fix an inverted test in random_rndr_read_one().