Re: locking against myself panic (cprng_strongreseed, filt_rndread)

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Wed, 8 Nov 2017 11:22:26 +0100
> From: Edgar Fuß <ef%math.uni-bonn.de@localhost>
> 
> > 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;
 }