Re: locking against myself panic (cprng_strongreseed, filt_rndread)

[Taylor R Campbell]

> Date: Wed, 8 Nov 2017 11:22:26 +0100
> From: Edgar Fuß <e...@math.uni-bonn.de>
> 
> > Not surprising: cprng locking was completely hosed in netbsd-6 until
> > it got rewritten for netbsd-7.
> So I can expect all of my servers to panic at any time?
> Can I mitigate the probability of the panic?

If you can guarantee either

(a) you have a hardware RNG, or
(b) the system is updating and using the /var/db/entropy file,

then you can safely replace /dev/random by a symlink to /dev/urandom,
and that should avoid the panic for certain use patterns.

Specifically, processes that first try to read from /dev/random, and
then wait with select/poll/kqueue only if it returns EAGAIN, will
avoid triggering that code path by reading exclusively from (what is
actually) /dev/urandom, which never returns EAGAIN.

Processes that start by select/poll/kqueue, however, may still trigger
the bad code path.  I threw together a patch, attached, that _might_
fix this particular code path, but (a) I don't have time to test or
even compile-test it at the moment, and (b) there are lots of other
things wrong with the netbsd-6 cprng stuff, so don't get too excited.

If you'd like to test it, we can see about applying it as a `pullup'
(not that it has any connection to the changes in netbsd-7 that were a
near-total rewrite).

diff --git a/sys/dev/rndpseudo.c b/sys/dev/rndpseudo.c
index 5501263020d5..68eeaa700d3b 100644
--- a/sys/dev/rndpseudo.c
+++ b/sys/dev/rndpseudo.c
@@ -673,13 +673,13 @@ rnd_poll(struct file *fp, int events)
                }       
        }
 
+       mutex_enter(&ctx->cprng->mtx);
        if (cprng_strong_ready(ctx->cprng)) {
                revents |= events & (POLLIN | POLLRDNORM);
        } else {
-               mutex_enter(&ctx->cprng->mtx);
                selrecord(curlwp, &ctx->cprng->selq);
-               mutex_exit(&ctx->cprng->mtx);
        }
+       mutex_exit(&ctx->cprng->mtx);
 
        return (revents);
 }
@@ -731,12 +731,24 @@ static int
 filt_rndread(struct knote *kn, long hint)
 {
        cprng_strong_t *c = kn->kn_hook;
+       int ret;
 
+       if (hint & NOTE_SUBMIT)
+               KASSERT(mutex_owned(&c->mtx));
+       else
+               mutex_enter(&c->mtx);
        if (cprng_strong_ready(c)) {
                kn->kn_data = RND_TEMP_BUFFER_SIZE;
-               return 1;
+               ret = 1;
+       } else {
+               ret = 0;
        }
-       return 0;
+       if (hint & NOTE_SUBMIT)
+               KASSERT(mutex_owned(&c->mtx));
+       else
+               mutex_exit(&c->mtx);
+
+       return ret;
 }
 
 static const struct filterops rnd_seltrue_filtops =
diff --git a/sys/kern/subr_cprng.c b/sys/kern/subr_cprng.c
index 168a83066844..cdb382b84c81 100644
--- a/sys/kern/subr_cprng.c
+++ b/sys/kern/subr_cprng.c
@@ -95,7 +95,7 @@ cprng_strong_doreseed(cprng_strong_t *const c)
        if (c->flags & CPRNG_USE_CV) {
                cv_broadcast(&c->cv);
        }
-       selnotify(&c->selq, 0, 0);
+       selnotify(&c->selq, 0, NOTE_SUBMIT);
 }
 
 static void
@@ -397,7 +397,7 @@ cprng_strong_setflags(cprng_strong_t *const c, int flags)
                        if (c->flags & CPRNG_USE_CV) {
                                cv_broadcast(&c->cv);
                        }
-                       selnotify(&c->selq, 0, 0);
+                       selnotify(&c->selq, 0, NOTE_SUBMIT);
                }
        }
        c->flags = flags;
diff --git a/sys/sys/cprng.h b/sys/sys/cprng.h
index 984a06fb1b11..278ca3ab2885 100644
--- a/sys/sys/cprng.h
+++ b/sys/sys/cprng.h
@@ -121,12 +121,11 @@ static inline int
 cprng_strong_ready(cprng_strong_t *c)
 {
        int ret = 0;
-       
-       mutex_enter(&c->mtx);
+
+       KASSERT(mutex_owned(&c->mtx));
        if (c->drbg.reseed_counter < NIST_CTR_DRBG_RESEED_INTERVAL) {
                ret = 1;
        }
-       mutex_exit(&c->mtx);
        return ret;
 }