[PATCH 3/4] urngd: Add support for read()ing entropy

[Nathaniel Filardo]

[resending with correct recipients; sorry for duplication]

On Mon, Jul 13, 2020 at 3:53 PM Petr Štetiar <ynezz at true.cz> wrote:
>
> nwfilardo at gmail.com <nwfilardo at gmail.com> [2020-07-13 10:08:17]:
>
> > From: Nathaniel Wesley Filardo <nwfilardo at 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 <nwfilardo at 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? :-)

Yes; the intent of checking in init was just to log a warning.  Having done so,
there's no point in checking again.

> 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.

Yes, calling read() would be semantically equivalent, but calling read() is way,
way more expensive than a function and an alloca() that you ask me to avoid
below.  I think it's not that much code to avoid the expense of a system call
for devices that may only occasionally be able to feed entropy to the system.

> 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.

OK.

> I would rather exchange those comments with handling of ent < 0 case, probably
> for the start DEBUG() out errors other then EAGAIN/EINTR etc.

Sorry, I don't think I understand that.

> > +     }
> > +
> > +     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);

OK.

> in order to avoid wasting cycles with alloca() call and reuse
> `entropy_device_available` in other places to make code more readable.

That's fine, but the cost of a function call and alloca() is tens of cycles,
since the alloca is just a subtraction of the stack pointer which a good
optimizing compiler can fuse into the allocation of local variables,
while a read() is more likely to be thousands.

> 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 :)

Of "device" and "file", I'd prefer "file" since "everything's a file on UNIX"
and "-d" is already taken (thus "-f").

> > +}
> > +
> > +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?

OK, I'll make it log a warning and not be fatal.

> That block is not oneliner so move that into separate function.

OK.

> > +                     }
> > +                     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.

jitter is guaranteed to be available, while the device hasn't been asked.
I don't think it much matters: if we're low on entropy we'll ask the device
almost immediately in the event loop anyway.

But if you'd prefer, I can call gather_file_entropy() at startup and use
that to arm the uloop_fd.

Cheers,
--nwf;