Re: [RFC PATCH 1/4] random: introduce getrandom() GRND_TIMESTAMP

[Peter Lafreniere]

[...]

>GRND_TIMESTAMP allows userspace to ask the kernel if previous
>"timestamp" has changed as the result of an event that
>triggered kernel CSPRNG reseed, and to update the "timestamp".

>In case the "timestamp" hasn't changed, userspace CSPRNG can
>consume a slice of its buffered random stream.

>If it has changed, remaining userspace buffered random values
>should be discarded. Userspace should call getrandom() to fill
>and/or generate its buffer with updated seed.
>It's advised to test again the "timestamp" to deal with the
>race condition, where the kernel reseed just after the call
>to getrandom() to get entropy.

This second check is the 'safe thing' to do, but if you're that
worried about race conditions then this api is useless. You can't
ignore the inherent TOCTOU problem with the GRND_TIMESTAMP calls.

That said, the race condition is so tiny that the added overhead
of a third syscall (without vDSO support) starts to negate the
value in buffering the random numbers (not to mention adding
significant latency to every nth arc4random() call, for example).

IMO, for callers that cannot accept the risk, the getrandom(,,0)
option is the perfect alternative.

[...]

>+static ssize_t get_random_timestamp(char __user *ubuf, size_t len, unsigned int flags)
>+{
>+	u64 ts;
>+
>+	/* other combination not supported */
>+	if (WARN(flags != GRND_TIMESTAMP, "GRND_TIMESTAMP cannot be used with other flags"))
>+		return -EINVAL;

If userspace messes up the flags then it's the problem of the
caller. Why clutter the logs in that case?

At the very least this should be WARN_ONCE() to avoid log spam.

>+	/* shorter structure not supported */
>+	if (len < sizeof(ts))
>+		return -EINVAL;

This should be sizeof(u64) to match the vDSO patch and to avoid
having to change this condition if ts becomes larger.

>+
>+	if (copy_from_user(&ts, ubuf, sizeof(ts)))
>+		return -EFAULT;
>+
>+	/* longer structure supported, only if 0-padded,
>+	   timestamp might be extended in the future with more fields */
>+	if (len > sizeof(ts)) {
>+		char __user *p = ubuf + sizeof(ts);
>+		size_t l = len - sizeof(ts);
>+
>+		while (l) {
>+			char b;
>+
>+			if (get_user(b, p++))
>+				return -EFAULT;
>+
>+			if (b)
>+				return -EINVAL;
>+		}
>+	}
>+
>+	if (!get_random_timestamp_update(&ts, READ_ONCE(base_crng.generation)))
>+		return 0;
>+
>+	if (copy_to_user(ubuf, &ts, sizeof(ts)))
>+		return -EFAULT;
>+
>+	return sizeof(ts);
>+}
>+
> SYSCALL_DEFINE3(getrandom, char __user *, ubuf, size_t, len, unsigned int, flags)
> {
> 	struct iov_iter iter;
> 	struct iovec iov;
> 	int ret;
>
>-	if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE))
>+	if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE | GRND_TIMESTAMP))
> 		return -EINVAL;
>
>+	if (unlikely(flags & GRND_TIMESTAMP))
>+		return get_random_timestamp(ubuf, len, flags);
>+

I'd remove the unlikely(). I don't like to assume usage patterns.

> 	/*
> 	 * Requesting insecure and blocking randomness at the same time makes
> 	 * no sense.
>diff --git a/include/linux/random.h b/include/linux/random.h
>index b0a940af4fff..bc219b5a96a5 100644
>--- a/include/linux/random.h
>+++ b/include/linux/random.h
>@@ -161,4 +161,35 @@ int random_online_cpu(unsigned int cpu);
> extern const struct file_operations random_fops, urandom_fops;
> #endif
>
>+/*
>+ * get_random_timestamp_update()
>+ *
>+ * @generation: current CRNG generation (from base_crng.generation
>+ *              or _vdso_rng_data.generation)
>+ *
>+ * Return: timestamp size if previous timestamp is expired and is updated,
>+ *         0 if not expired (and not updated)
>+ */
>+static inline bool get_random_timestamp_update(u64 *user_ts,
>+					       u64 generation)
>+{
>+	u64 ts;
>+
>+	/* base_crng.generation is never ULONG_MAX,
>+	 * OTOH userspace will initialize its timestamp
>+	 * to 0, so inverting base_crng.generation ensure
>+	 * first call to getrandom(,,GRND_TIMESTAMP) will
>+	 * update
>+	 */

Rather than assuming that the timestamp will start zero-initilized,
expect it to be uninitilized. Either way the code works.

>+	ts = ~generation;
>+
>+	/* not expired ? no refresh suggested */
>+	if (*user_ts == ts)
>+		return false;
>+
>+	*user_ts = ts;
>+
>+	return true;
>+}
>+

Not that it matters much, but you could make generation a u64* that
gets dereferenced by get_random_timestamp_update(). It's cleaner for
the caller, barely changes this function, and will get inlined anyway.
I might just be imposing my personal style in this case though.

After a cursory skimming of the rest of the series I think that this
is a worthwhile direction to pursue. Jason's series is growing bulky
and this provides the needed slimming while solving the root problem.

The only thing I see immediately is the TOCTOU problem and the extra
steps needed to guarantee forward secrecy.

I should mention that I'm not a security or rng expert at all.

Cheers,
Peter Lafreniere
<peter@xxxxxxxx>