Re: Implementing getrandom/getentropy, anybody?

[Andrew Eggenberger]

Thanks for the feedback Samuel and Gillem. I've
incorporated it into the new patch. I'm not sure
if the call to __read_nocancel needs to be
tested because read returns -1 on error anyway.

Samuel, I plan to send the copyright assignment form tomorrow.

Thanks.
Andrew

>From 9357903b7d22ea1b54fb3c786e82b8fd0d08d996 Mon Sep 17 00:00:00 2001
From: Andrew Eggenberger 
Date: Tue, 29 Oct 2019 23:19:32 -0500
Subject: [PATCH] add getrandom and getentropy implementations

squash! add getrandom and getentropy implementations
---
 sysdeps/mach/hurd/getentropy.c | 65 ++
 sysdeps/mach/hurd/getrandom.c  | 50 ++
 2 files changed, 115 insertions(+)
 create mode 100644 sysdeps/mach/hurd/getentropy.c
 create mode 100644 sysdeps/mach/hurd/getrandom.c

diff --git a/sysdeps/mach/hurd/getentropy.c b/sysdeps/mach/hurd/getentropy.c
new file mode 100644
index 00..ae2e6440da
--- /dev/null
+++ b/sysdeps/mach/hurd/getentropy.c
@@ -0,0 +1,65 @@
+/* Implementation of getentropy based on getrandom.
+   Copyright (C) 2016-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   .  */
+
+#include 
+#include 
+#include 
+#include 
+
+/* Write LENGTH bytes of randomness starting at BUFFER.  Return 0 on
+   success and -1 on failure.  */
+int
+getentropy (void *buffer, size_t length)
+{
+  /* The interface is documented to return EIO for buffer lengths
+ longer than 256 bytes.  */
+  if (length > 256)
+{
+  __set_errno (EIO);
+  return -1;
+}
+
+  /* Try to fill the buffer completely.  Even with the 256 byte limit
+ above, we might still receive an EINTR error (when blocking
+ during boot).  */
+  void *end = buffer + length;
+  while (buffer < end)
+{
+  /* NB: No cancellation point.  */
+  ssize_t bytes = getrandom(buffer, end - buffer, 0);
+  if (bytes < 0)
+{
+  if (errno == EINTR)
+/* Try again if interrupted by a signal.  */
+continue;
+  else
+return -1;
+}
+  if (bytes == 0)
+{
+  /* No more bytes available.  This should not happen under
+ normal circumstances.  */
+  __set_errno (EIO);
+  return -1;
+}
+  /* Try again in case of a short read.  */
+  buffer += bytes;
+}
+  return 0;
+}
+
diff --git a/sysdeps/mach/hurd/getrandom.c b/sysdeps/mach/hurd/getrandom.c
new file mode 100644
index 00..a65b33a475
--- /dev/null
+++ b/sysdeps/mach/hurd/getrandom.c
@@ -0,0 +1,50 @@
+/* Hurdish implementation of getrandom
+   Copyright (C) 2016-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   .  */
+
+#include 
+#include 
+#include 
+#include 
+
+/* Write up to LENGTH bytes of randomness starting at BUFFER.
+   Return the number of bytes written, or -1 on error.  */
+ssize_t
+getrandom (void *buffer, size_t length, unsigned int flags)
+{
+  const char *random_source = "/dev/urandom";
+  size_t amount_read;
+  int fd, err;
+
+  if (flags & GRND_RANDOM){
+random_source = "/dev/random";
+  }
+
+  fd = __open_nocancel(random_source, O_CLOEXEC);
+  if (fd == -1){
+return -1;
+  }
+  amount_read = __read_nocancel(fd, buffer, length);
+  if (amount_read == -1){
+return -1;
+  }
+  err = __close_nocancel(fd);
+  if (err == -1){
+return -1;
+  }
+  return amount_read;
+}
-- 
2.23.0

Re: Implementing getrandom/getentropy, anybody?

[Samuel Thibault]

Hello,

Thanks Guillem for the additional review :)

Guillem Jover, le jeu. 31 oct. 2019 13:31:58 +0100, a ecrit:
> On Tue, 2019-10-29 at 23:28:26 -0500, Andrew Eggenberger wrote:
> > +  if (flags & GRND_RANDOM){
> > +random_source = "/dev/random";
> > +  }
> > +
> > +  fp = open(random_source, O_RDONLY);
> 
> Shouldn't this be opened with O_CLOEXEC, otherwise children created
> by other threads might leak file descriptors. Although I don't see
> this being consistently done in glibc, not sure why?

Indeed.  I guess it's not always done only due to historical reasons.

> > +  amount_read = read(fp, buffer, length);
> 
> What about partial reads?

That is fine, the getrandom interface explicitly allows partial reads.

Samuel

Re: Implementing getrandom/getentropy, anybody?

[Guillem Jover]

Hi!

On Tue, 2019-10-29 at 23:28:26 -0500, Andrew Eggenberger wrote:
> --- /dev/null
> +++ b/sysdeps/mach/hurd/getrandom.c

> +/* Write up to LENGTH bytes of randomness starting at BUFFER.
> +   Return the number of bytes written, or -1 on error.  */
> +ssize_t
> +getrandom (void *buffer, size_t length, unsigned int flags)
> +{
> +  char* random_source = "/dev/urandom";
> +  int amount_read, fp;

I'd define amount_read as ssize_t. I'd probably call this fd, fp is
usually associated with a «FILE *». Shouldn't the * be next to the
variable instead of the type?

> +  if (flags & GRND_RANDOM){
> +random_source = "/dev/random";
> +  }
> +
> +  fp = open(random_source, O_RDONLY);

Shouldn't this be opened with O_CLOEXEC, otherwise children created
by other threads might leak file descriptors. Although I don't see
this being consistently done in glibc, not sure why?

> +  amount_read = read(fp, buffer, length);

What about partial reads?

> +  close(fp);
> +  return amount_read;
> +}

Thanks,
Guillem