Page MenuHomeFreeBSD

arc4random: Avoid KMSAN false positives from pre-seeding results
ClosedPublic

Authored by markj on Aug 11 2021, 8:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 14 2022, 3:38 AM
Unknown Object (File)
Dec 12 2022, 2:16 AM
Unknown Object (File)
Dec 12 2022, 2:12 AM
Unknown Object (File)
Dec 12 2022, 2:04 AM
Subscribers

Details

Summary

If code calls arc4random(), and our RNG is not yet seeded and
random_bypass_before_seeding is true, we'll compute a key using the
SHA256 hash of some hopefully hard-to-predict data, including the
contents of an uninitialized stack buffer (which is also the output
buffer).

When KMSAN is enabled, this use of uninitialized state propagtes through
to the arc4random() output, resulting in false positives. To fix this,
lie to KMSAN and explicitly mark the buffer as initialized.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Aug 11 2021, 8:07 PM

It's not exactly a false positive, although poisoning the output as uninitialized is sort of unhelpful. Are we confident the compiler isn't eliminating access to it (due to UB) outside of KMSAN? In the abstract, I think we would prefer to eliminate bypass_before_seeding.

I'd probably add some summarized form of the commit message as a comment adjacent to the kmsan_mark() invocation.

This revision is now accepted and ready to land.Aug 11 2021, 11:05 PM
In D31510#710348, @cem wrote:

It's not exactly a false positive, although poisoning the output as uninitialized is sort of unhelpful.

Yeah, everything is working as expected here, I've just been tending to use "false positive" to mean, "any KMSAN report that doesn't correspond to a code bug."

Are we confident the compiler isn't eliminating access to it (due to UB) outside of KMSAN? In the abstract, I think we would prefer to eliminate bypass_before_seeding.

What exactly is the UB here? I'm not sure how the compiler could legally eliminate SHA256_Update(&ctx, key, sizeof(key)).

I'd probably add some summarized form of the commit message as a comment adjacent to the kmsan_mark() invocation.

Will do, thanks.

What exactly is the UB here? I'm not sure how the compiler could legally eliminate SHA256_Update(&ctx, key, sizeof(key)).

Any use of uninitialized variables is UB, I think?

In D31510#710613, @cem wrote:

What exactly is the UB here? I'm not sure how the compiler could legally eliminate SHA256_Update(&ctx, key, sizeof(key)).

Any use of uninitialized variables is UB, I think?

I believe so, I just don't see how the compiler can infer that key is being accessed here.

LTO builds can see across CUs. I don’t know of any particular pass that would eliminate this, though.