Re: use of cprng_fast vs. random(9) (Was: CVS commit: src/sys/altq)

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Wed, 22 Jan 2025 15:03:36 +0100
> From: Christoph Badura <bad%bsd.de@localhost>
> 
> So, I don't understand this changes and the followup change by kre.
> 
> Why, exactly, do we need "a more secure random generator" to randomly drop
> packets from a queue?  What was the actual complaint by coverity and why
> wasn't it a false positive?

Coverity probably just doesn't like the standard C random() function,
generally.  Which is a reasonable objection, generally: almost
everyone is better served by cryptographic RNG by default, unless
there is a compelling performance reason for overriding that default.

The NetBSD kernel function random() is essentially the same concept as
the standard C one, and has not been replaced by a wrapper around
cprng_fast, mainly because I didn't understand the performance
implications of the handful of existing users of random() in the
kernel well enough to be confident in the safety of this replacement
when I looked into doing this.  (E.g., if they run in interrupt
context, needs extra work.)

(How could an adversary take advantage of predicatble random() in
altq?  Potentially by predicting the sequence of drops in order to
deny service to legitimate traffic with slightly less bandwidth.
Probably hard to pull off, but then I'm not an attacker so I'm not
motivated to try.)

> The followup commit by kre partiallly undid the change for "rump and
> anything else userland".  Why is this necessary for rump?

This change was not necessary for rump, and doesn't even affect rump,
because rump is built with _KERNEL defined -- just not _HARDKERNEL.

> AFAICT the kernel rng subsystem works just fine in rump servers, so
> presumably cprng_fast* is available there.  Hence it should be possible to
> fix this correctly instead of pampering over the real issue.[1]
> 
> At the very least I would have expected a comment in the code why using
> cprng_fast32 in rump kernels is not possible.  And what specifically the
> "anything else userland" is.

Here's the actual failure:

--- sanitizer_platform_limits_netbsd.d ---
In file included from /tmp/build/2025.01.18.16.51.28-i386/destdir/usr/include/altq/altq_blue.h:33,
                 from /tmp/build/2025.01.18.16.51.28-i386/src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_platform_limits_netbsd.cpp:107:
/tmp/build/2025.01.18.16.51.28-i386/destdir/usr/include/altq/altq_classq.h:42:10: fatal error: sys/cprng.h: No such file or directory
   42 | #include <sys/cprng.h>
      |          ^~~~~~~~~~~~~

https://releng.netbsd.org/b5reports/i386/2025/2025.01.18.16.51.28/build.log.tail

Why this includes altq_classq.h -- and why altq_classq.h has extern
"C" when built as C++, around no declarations -- is unclear to me.  It
comes in only transitively from other altq header files.

The inner #ifdef _KERNEL should go away because it's redundant (it's
inside an existing #ifdef _KERNEL) and the #include <sys/cprng.h> can
just be put inside the outer #ifdef _KERNEL.