[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 9 Apr 2022 09:18:17 -0400
From: Rich Felker <dalias@...c.org>
To: "Jason A. Donenfeld" <Jason@...c4.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] getentropy: fail if buffer not completely filled
On Sat, Apr 09, 2022 at 02:10:47AM +0200, Jason A. Donenfeld wrote:
> The man page for getentropy says that it either completely succeeds or
> completely fails, and indeed this is what glibc does. However, musl has
> a condition where it breaks out of the loop early, yet still returns a
> success. This patch fixes that by returning a success only if the buffer
> is completely filled. While we're at it, prevent an unexpected infinite
> loop if the function returns 0, the same way glibc does, just in case.
> ---
> src/misc/getentropy.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/misc/getentropy.c b/src/misc/getentropy.c
> index 651ea95f..5b2fc7a1 100644
> --- a/src/misc/getentropy.c
> +++ b/src/misc/getentropy.c
> @@ -6,7 +6,7 @@
>
> int getentropy(void *buffer, size_t len)
> {
> - int cs, ret = 0;
> + int cs, ret;
> char *pos = buffer;
>
> if (len > 256) {
> @@ -18,16 +18,19 @@ int getentropy(void *buffer, size_t len)
>
> while (len) {
> ret = getrandom(pos, len, 0);
> - if (ret < 0) {
> + if (ret <= 0) {
> if (errno == EINTR) continue;
> else break;
> }
> pos += ret;
> len -= ret;
> - ret = 0;
> }
>
> pthread_setcancelstate(cs, 0);
>
> - return ret;
> + if (len) {
> + errno = EIO;
> + return -1;
> + }
> + return 0;
> }
> --
> 2.35.1
We already seem to cover the case (which should not be possible, per
the getrandom syscall contract) where getrandom returns fewer than 256
bytes, and the one where it fails with EINTR due to interruption by a
signal before the urandom pool was initialized. It looks to me like
you're positing a case where the syscall returns 0, but that's not
something that can happen. A failure to read any bytes when 0 wasn't
the length requested is always an error of some sort (EAGAIN, EINTR,
etc.) not a short read of length zero.
As written the patch also has at least one bug, checking errno when a
non-error value (0) was returned.
I do want to enhance this function not to be able to fail at all
except on invalid input (the existing EIO case). This requires use of
the legacy SYS_sysctl syscall which is deprecated and removed from
modern kernels, but all the kernel versions (AIUI back to way before
supportable versions) lacking SYS_getrandom had it.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
