nwfila...@gmail.com <nwfila...@gmail.com> [2020-07-13 10:08:17]:
> From: Nathaniel Wesley Filardo <nwfila...@gmail.com>
>
> This allows us to attach a hwrng, for example.
>
> At most one sample will be taken every time we also add entropy via the
> jitter mechanism. We won't try reading if the stream hasn't indicated
> its readiness, and we'll go back to waiting if ever the stream produces
> 0 bytes or an error on read.
>
> Signed-off-by: Nathaniel Wesley Filardo <nwfila...@gmail.com>
> ---
> urngd.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/urngd.c b/urngd.c
> index 31181f0..d6c48f8 100644
> --- a/urngd.c
> +++ b/urngd.c
> @@ -61,6 +61,7 @@ unsigned int debug;
>
> struct urngd {
> struct uloop_fd rnd_fd;
> + struct uloop_fd src_fd;
> struct rand_data *ec;
> };
>
> @@ -88,7 +89,7 @@ static size_t write_entropy(struct urngd *u, struct
> rand_pool_info *rpi)
> return ret;
> }
>
> -static size_t gather_entropy(struct urngd *u)
> +static size_t gather_jitter_entropy(struct urngd *u)
> {
> ssize_t ent;
> size_t ret = 0;
> @@ -110,12 +111,48 @@ static size_t gather_entropy(struct urngd *u)
> return ret;
> }
>
> +static size_t gather_src_entropy(struct urngd *u) {
> + static const size_t src_bytes = 1024;
> + struct rand_pool_info *rpi = alloca(sizeof(*rpi) + src_bytes);
> + ssize_t ent;
> + size_t ret;
> +
> + if ((u->src_fd.fd < 0) || (u->src_fd.registered)) {
> + /* No source or source still waiting for available bytes */
> + return 0;
> + }
> +
> + ent = read(u->src_fd.fd, (char *)&rpi->buf[0], src_bytes);
> + if (ent > 0) {
> + /* Read some bytes from the source; stir those in, too */
> + rpi->buf_size = ent;
> + rpi->entropy_count = 8 * ent;
> + ret = write_entropy(u, rpi);
> + } else {
> + /* No luck this time around */
> + ret = 0;
> +
> + /* Go back to waiting for the source to be ready */
> + uloop_fd_add(&u->src_fd, ULOOP_READ);
in init you check the return value, but now don't care? :-) Anyway, I think,
that using uloop for this readiness purpose is overkill, wouldn't just read()
suffice here? Too much code with no added value.
Too much comments as well, I think, that it's pretty obvious what it is doing.
If not, then please move that code into separate properly (self-documenting)
named function.
I would rather exchange those comments with handling of ent < 0 case, probably
for the start DEBUG() out errors other then EAGAIN/EINTR etc.
> + }
> +
> + memset_secure(&rpi->buf, 0, ent);
> +
> + return ret;
> +}
> +
> static void low_entropy_cb(struct uloop_fd *ufd, unsigned int events)
> {
> struct urngd *u = container_of(ufd, struct urngd, rnd_fd);
>
> DEBUG(2, DEV_RANDOM " signals low entropy\n");
> - gather_entropy(u);
> + gather_jitter_entropy(u);
> + gather_src_entropy(u);
I would rather do:
static inline bool entropy_device_available(struct urngd *u)
{
return u->fd >= 0;
}
if (entropy_device_available(u))
gather_device_entropy(u);
in order to avoid wasting cycles with alloca() call and reuse
`entropy_device_available` in other places to make code more readable.
That being said I find that `source` name ambiguous and would prefer `device`
instead as it's actually RNG device behind that FD (perhaps `file` would
work as well)
Naming is hard :)
> +}
> +
> +static void src_ready_cb(struct uloop_fd *ufd, unsigned int events)
> +{
> + uloop_fd_delete(ufd);
> }
>
> static void urngd_done(struct urngd *u)
> @@ -129,6 +166,11 @@ static void urngd_done(struct urngd *u)
> close(u->rnd_fd.fd);
> u->rnd_fd.fd = 0;
> }
> +
> + if (u->src_fd.fd >= 0) {
> + close(u->src_fd.fd);
> + u->src_fd.fd = -1;
> + }
> }
>
> static bool urngd_init(struct urngd *u)
> @@ -154,6 +196,18 @@ static bool urngd_init(struct urngd *u)
>
> uloop_fd_add(&u->rnd_fd, ULOOP_WRITE);
>
> + if (u->src_fd.fd >= 0) {
> + int ret;
> +
> + u->src_fd.cb = src_ready_cb;
> + ret = uloop_fd_add(&u->src_fd, ULOOP_READ);
> + if (ret == -1 && errno == EPERM) {
> + LOG("Source (-f) does not support polling;"
> + " assuming that's OK.");
> + u->src_fd.registered = false;
> + }
> + }
> +
> return true;
> }
>
> @@ -164,6 +218,7 @@ static int usage(const char *prog)
> #ifdef URNGD_DEBUG
> " -d <level> Enable debug messages\n"
> #endif
> + " -f <file> Source entropy from <file>\n"
> " -S Print messages to stdout\n"
> "\n", prog);
> return 1;
> @@ -182,13 +237,24 @@ int main(int argc, char **argv)
> }
> #endif
>
> - while ((ch = getopt(argc, argv, "d:S")) != -1) {
> + urngd_service.src_fd.fd = -1;
> +
> + while ((ch = getopt(argc, argv, "d:f:S")) != -1) {
> switch (ch) {
> #ifdef URNGD_DEBUG
> case 'd':
> debug = atoi(optarg);
> break;
> #endif
> + case 'f':
> + urngd_service.src_fd.fd =
> + open(optarg, O_RDONLY | O_NONBLOCK);
> + if (urngd_service.src_fd.fd < 0) {
> + ERROR("%s open failed: %s\n",
> + optarg, strerror(errno));
> + return -1;
Jitter entropy is probably still usable, still better then no entropy, right?
That block is not oneliner so move that into separate function.
> + }
> + break;
> case 'S':
> ulog_channels = ULOG_STDIO;
> break;
> @@ -207,7 +273,7 @@ int main(int argc, char **argv)
>
> LOG("v%s started.\n", URNGD_VERSION);
>
> - gather_entropy(&urngd_service);
> + gather_jitter_entropy(&urngd_service);
Why just jitter source? I would like to get entropy from device as well if
available.
> uloop_run();
> uloop_done();
> --
> 2.27.0
>
--
ynezz
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
