Page MenuHomeFreeBSD
Differential D19744 Diff 56205 sys/dev/random/randomdev.c

Changeset View

Standalone View

View Options

sys/dev/random/randomdev.c

Show First 20 Lines • Show All 57 Lines • ▼ Show 20 Lines
#include <dev/random/random_harvestq.h> #include <dev/random/random_harvestq.h>
#define RANDOM_UNIT 0 #define RANDOM_UNIT 0
#if defined(RANDOM_LOADABLE) #if defined(RANDOM_LOADABLE)
#define READ_RANDOM_UIO _read_random_uio #define READ_RANDOM_UIO _read_random_uio
#define READ_RANDOM _read_random #define READ_RANDOM _read_random
static int READ_RANDOM_UIO(struct uio *, bool); static int READ_RANDOM_UIO(struct uio *, bool);
static u_int READ_RANDOM(void *, u_int); static void READ_RANDOM(void *, u_int);
#else #else
#define READ_RANDOM_UIO read_random_uio #define READ_RANDOM_UIO read_random_uio
#define READ_RANDOM read_random #define READ_RANDOM read_random
#endif #endif
static d_read_t randomdev_read; static d_read_t randomdev_read;
static d_write_t randomdev_write; static d_write_t randomdev_write;
static d_poll_t randomdev_poll; static d_poll_t randomdev_poll;
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines
/* ARGSUSED */ /* ARGSUSED */
static int static int
randomdev_read(struct cdev *dev __unused, struct uio *uio, int flags) randomdev_read(struct cdev *dev __unused, struct uio *uio, int flags)
{ {
return (READ_RANDOM_UIO(uio, (flags & O_NONBLOCK) != 0)); return (READ_RANDOM_UIO(uio, (flags & O_NONBLOCK) != 0));
} }
int /*
READ_RANDOM_UIO(struct uio *uio, bool nonblock) * If the random device is not seeded, blocks until it is seeded.
*
* Returns zero when the random device is seeded.
*
* If the 'interruptible' parameter is true, and the device is unseeded, this
* routine may be interrupted. If interrupted, it will return either ERESTART
* or EINTR.
*/
#define SEEDWAIT_INTERRUPTIBLE true
#define SEEDWAIT_UNINTERRUPTIBLE false
static int
randomdev_wait_until_seeded(bool interruptible)
{ {
uint8_t *random_buf; int error, spamcount, slpflags;
int error, spamcount;
ssize_t read_len, total_read, c;
/* 16 MiB takes about 0.08 s CPU time on my 2017 AMD Zen CPU */
#define SIGCHK_PERIOD (16 * 1024 * 1024)
const size_t sigchk_period = SIGCHK_PERIOD;
CTASSERT(SIGCHK_PERIOD % PAGE_SIZE == 0); slpflags = interruptible ? PCATCH : 0;
#undef SIGCHK_PERIOD
random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
p_random_alg_context->ra_pre_read();
error = 0; error = 0;
spamcount = 0; spamcount = 0;
/* (Un)Blocking logic */
while (!p_random_alg_context->ra_seeded()) { while (!p_random_alg_context->ra_seeded()) {
if (nonblock) {
error = EWOULDBLOCK;
break;
}
/* keep tapping away at the pre-read until we seed/unblock. */ /* keep tapping away at the pre-read until we seed/unblock. */
p_random_alg_context->ra_pre_read(); p_random_alg_context->ra_pre_read();
/* Only bother the console every 10 seconds or so */ /* Only bother the console every 10 seconds or so */
if (spamcount == 0) if (spamcount == 0)
printf("random: %s unblock wait\n", __func__); printf("random: %s unblock wait\n", __func__);
spamcount = (spamcount + 1)%100; spamcount = (spamcount + 1) % 100;
error = tsleep(&random_alg_context, PCATCH, "randseed", hz/10); error = tsleep(&random_alg_context, slpflags, "randseed",
if (error == ERESTART || error == EINTR) hz / 10);
if (error == ERESTART || error == EINTR) {
KASSERT(interruptible,
("unexpected wake of non-interruptible sleep"));
break; break;
}
/* Squash tsleep timeout condition */ /* Squash tsleep timeout condition */
if (error == EWOULDBLOCK) if (error == EWOULDBLOCK)
error = 0; error = 0;
KASSERT(error == 0, ("unexpected tsleep error %d", error)); KASSERT(error == 0, ("unexpected tsleep error %d", error));
} }
return (error);
}
int
READ_RANDOM_UIO(struct uio *uio, bool nonblock)
{
delphijUnsubmitted
Done
Inline Actions

Instead of allowing the caller to specify unsupported flags and assert it later, it's better to just make this 'bool interruptable' and translate it to the only supported flag instead. Additionally you can probably name this something like: randomdev_waitseed_with_interruptable()?

delphij: Instead of allowing the caller to specify unsupported flags and assert it later, it's better to…
cemAuthorUnsubmitted
Done
Inline Actions

Instead of allowing the caller to specify unsupported flags and assert it later, it's better to just make this 'bool

I'm not sure that's generally true. It may be the case that this is the only flag we ever anticipate, and in that case, bool would make sense.

In general, sometimes we end up adding additional flags to functions and it is useful to have the spare bits in an existing parameter. In the case where only some bits are defined, I think asserting valid input is a good thing.

I'm open to the argument that we'll never add additional flags to this function, and if you want to make that argument, I'm happy to convert to bool parameter. But I don't see that case made.

cem: > Instead of allowing the caller to specify unsupported flags and assert it later, it's better…
uint8_t *random_buf;
int error;
ssize_t read_len, total_read, c;
/* 16 MiB takes about 0.08 s CPU time on my 2017 AMD Zen CPU */
#define SIGCHK_PERIOD (16 * 1024 * 1024)
const size_t sigchk_period = SIGCHK_PERIOD;
CTASSERT(SIGCHK_PERIOD % PAGE_SIZE == 0);
#undef SIGCHK_PERIOD
delphijUnsubmitted
Done
Inline Actions

int sleep_flag = (interruptable)? PCATCH: 0;

delphij: int sleep_flag = (interruptable)? PCATCH: 0;
cemAuthorUnsubmitted
Done
Inline Actions

Sure; this is just the obvious way to implement the same behavior with a bool parameter instead of flags. I understand the idea and how it would be done; what's missing is the rationale. :-)

cem: Sure; this is just the obvious way to implement the same behavior with a bool parameter instead…
random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
p_random_alg_context->ra_pre_read();
error = 0;
/* (Un)Blocking logic */
if (!p_random_alg_context->ra_seeded()) {
if (nonblock)
error = EWOULDBLOCK;
else
delphijUnsubmitted
Done
Inline Actions

sleep_flag

delphij: sleep_flag
error = randomdev_wait_until_seeded(
SEEDWAIT_INTERRUPTIBLE);
}
delphijUnsubmitted
Done
Inline Actions

KASSERT(interruptable, ...

delphij: KASSERT(interruptable, ...
if (error == 0) { if (error == 0) {
read_rate_increment((uio->uio_resid + sizeof(uint32_t))/sizeof(uint32_t)); read_rate_increment((uio->uio_resid + sizeof(uint32_t))/sizeof(uint32_t));
total_read = 0; total_read = 0;
while (uio->uio_resid && !error) { while (uio->uio_resid && !error) {
read_len = uio->uio_resid; read_len = uio->uio_resid;
/* /*
* Belt-and-braces. * Belt-and-braces.
* Round up the read length to a crypto block size multiple, * Round up the read length to a crypto block size multiple,
* which is what the underlying generator is expecting. * which is what the underlying generator is expecting.
* See the random_buf size requirements in the Fortuna code. * See the random_buf size requirements in the Fortuna code.
*/ */
read_len = roundup(read_len, RANDOM_BLOCKSIZE); read_len = roundup(read_len, RANDOM_BLOCKSIZE);
/* Work in chunks page-sized or less */ /* Work in chunks page-sized or less */
read_len = MIN(read_len, PAGE_SIZE); read_len = MIN(read_len, PAGE_SIZE);
p_random_alg_context->ra_read(random_buf, read_len); p_random_alg_context->ra_read(random_buf, read_len);
c = MIN(uio->uio_resid, read_len); c = MIN(uio->uio_resid, read_len);
/* /*
* uiomove() may yield the CPU before each 'c' bytes * uiomove() may yield the CPU before each 'c' bytes
* (up to PAGE_SIZE) are copied out. * (up to PAGE_SIZE) are copied out.
*/ */
error = uiomove(random_buf, c, uio); error = uiomove(random_buf, c, uio);
total_read += c; total_read += c;
/* /*
* Poll for signals every few MBs to avoid very long * Poll for signals every few MBs to avoid very long
* uninterruptible syscalls. * uninterruptible syscalls.
delphijUnsubmitted
Done
Inline Actions

(true)

delphij: (true)
*/ */
if (error == 0 && uio->uio_resid != 0 && if (error == 0 && uio->uio_resid != 0 &&
total_read % sigchk_period == 0) { total_read % sigchk_period == 0) {
error = tsleep_sbt(&random_alg_context, PCATCH, error = tsleep_sbt(&random_alg_context, PCATCH,
"randrd", SBT_1NS, 0, C_HARDCLOCK); "randrd", SBT_1NS, 0, C_HARDCLOCK);
/* Squash tsleep timeout condition */ /* Squash tsleep timeout condition */
if (error == EWOULDBLOCK) if (error == EWOULDBLOCK)
error = 0; error = 0;
} }
} }
if (error == ERESTART || error == EINTR) if (error == ERESTART || error == EINTR)
error = 0; error = 0;
} }
free(random_buf, M_ENTROPY); free(random_buf, M_ENTROPY);
return (error); return (error);
} }
/*- /*-
* Kernel API version of read_random(). * Kernel API version of read_random().
* This is similar to random_alg_read(), * This is similar to random_alg_read(),
* except it doesn't interface with uio(9). * except it doesn't interface with uio(9).
* It cannot assumed that random_buf is a multiple of * It cannot assumed that random_buf is a multiple of
* RANDOM_BLOCKSIZE bytes. * RANDOM_BLOCKSIZE bytes.
*/ */
u_int void
READ_RANDOM(void *random_buf, u_int len) READ_RANDOM(void *random_buf, u_int len)
{ {
u_int read_len; u_int read_directly_len;
uint8_t local_buf[len + RANDOM_BLOCKSIZE];
delphijUnsubmitted
Done
Inline Actions

I'd probably rename this as "tail_buf" and keep the read_len name, see below.

delphij: I'd probably rename this as "tail_buf" and keep the read_len name, see below.
KASSERT(random_buf != NULL, ("No suitable random buffer in %s", __func__)); KASSERT(random_buf != NULL, ("No suitable random buffer in %s", __func__));
p_random_alg_context->ra_pre_read(); p_random_alg_context->ra_pre_read();
/* (Un)Blocking logic; if not seeded, return nothing. */ /* (Un)Blocking logic */
if (p_random_alg_context->ra_seeded()) { if (!p_random_alg_context->ra_seeded())
read_rate_increment((len + sizeof(uint32_t))/sizeof(uint32_t)); (void)randomdev_wait_until_seeded(SEEDWAIT_UNINTERRUPTIBLE);
if (len > 0) { read_rate_increment(roundup2(len, sizeof(uint32_t)));
if (len == 0)
return;
/* /*
* Belt-and-braces. * The underlying generator expects multiples of
* Round up the read length to a crypto block size multiple, * RANDOM_BLOCKSIZE.
* which is what the underlying generator is expecting.
*/ */
read_len = roundup(len, RANDOM_BLOCKSIZE); read_directly_len = rounddown(len, RANDOM_BLOCKSIZE);
delphijUnsubmitted
Done
Inline Actions

So basically this code block does was to read whole RANDOM_BLOCKSIZE's into the supplied buffer, and if the supplied 'len' is not multiple, the tail is filled with bytes from the on-stack buffer. This is a good change because it reduced the possibility that the data gets exposed through stack buffer, which is probably worth mentioning in the comments, by the way.

If I was you, I'd probably keep the existing read_len naming because it could be somewhat confusing what 'direct_len' means, though.

delphij: So basically this code block does was to read whole RANDOM_BLOCKSIZE's into the supplied buffer…
cemAuthorUnsubmitted
Done
Inline Actions

This is a good change because it reduced the possibility that the data gets exposed through stack buffer

The previous scheme also had the issue where a caller could induce kstack overflow by specifying arbitrarily large len. And this change avoids the extra copy as well. I can mention it in the commit message.

If stack exposure is still a (reduced) concern, perhaps we should explicit_bzero local_buf before returning?

I'd probably keep the existing read_len naming because it could be somewhat confusing what 'direct_len' means, though.

How about read_directly_len?

cem: > This is a good change because it reduced the possibility that the data gets exposed through…
delphijUnsubmitted
Done
Inline Actions

Sounds good, thanks!

delphij: Sounds good, thanks!
p_random_alg_context->ra_read(local_buf, read_len); if (read_directly_len > 0)
memcpy(random_buf, local_buf, len); p_random_alg_context->ra_read(random_buf, read_directly_len);
if (read_directly_len < len) {
uint8_t remainder_buf[RANDOM_BLOCKSIZE];
p_random_alg_context->ra_read(remainder_buf,
sizeof(remainder_buf));
memcpy((char *)random_buf + read_directly_len, remainder_buf,
len - read_directly_len);
explicit_bzero(remainder_buf, sizeof(remainder_buf));
} }
} else
len = 0;
return (len);
} }
static __inline void static __inline void
randomdev_accumulate(uint8_t *buf, u_int count) randomdev_accumulate(uint8_t *buf, u_int count)
{ {
static u_int destination = 0; static u_int destination = 0;
static struct harvest_event event; static struct harvest_event event;
static struct randomdev_hash hash; static struct randomdev_hash hash;
▲ Show 20 Lines • Show All 185 Lines • Show Last 20 Lines