mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] getentropy: fail if buffer not completely filled
@ 2022-04-09  0:10 Jason A. Donenfeld
  2022-04-09 13:18 ` Rich Felker
  0 siblings, 1 reply; 6+ messages in thread
From: Jason A. Donenfeld @ 2022-04-09  0:10 UTC (permalink / raw)
  To: musl; +Cc: Jason A. Donenfeld

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] [PATCH] getentropy: fail if buffer not completely filled
  2022-04-09  0:10 [musl] [PATCH] getentropy: fail if buffer not completely filled Jason A. Donenfeld
@ 2022-04-09 13:18 ` Rich Felker
  2022-04-09 22:50   ` Jason A. Donenfeld
  0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2022-04-09 13:18 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: musl

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] [PATCH] getentropy: fail if buffer not completely filled
  2022-04-09 13:18 ` Rich Felker
@ 2022-04-09 22:50   ` Jason A. Donenfeld
  2022-04-09 22:58     ` [musl] [PATCH v2] " Jason A. Donenfeld
  0 siblings, 1 reply; 6+ messages in thread
From: Jason A. Donenfeld @ 2022-04-09 22:50 UTC (permalink / raw)
  To: dalias; +Cc: musl

Hi Rich,

I can get rid of the <= case. glibc's paranoia might not actually make
sense. Actually, presumably one can just get rid of the entire loop,
assuming the < 256 guarantee continues to hold.

I'll send you two patches, one that moderately fixes up this v1
(called v2) and one that gets rid of the loop entirely (v3).

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [musl] [PATCH v2] getentropy: fail if buffer not completely filled
  2022-04-09 22:50   ` Jason A. Donenfeld
@ 2022-04-09 22:58     ` Jason A. Donenfeld
  2022-04-09 22:58       ` [musl] [PATCH v3] " Jason A. Donenfeld
  2022-04-10 15:30       ` [musl] [PATCH v2] " Rich Felker
  0 siblings, 2 replies; 6+ messages in thread
From: Jason A. Donenfeld @ 2022-04-09 22:58 UTC (permalink / raw)
  To: Rich Felker, musl; +Cc: Jason A. Donenfeld

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.
---
Changes v2->v3:
- This gets rid of the ret==0 check like glibc uses.

 src/misc/getentropy.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/misc/getentropy.c b/src/misc/getentropy.c
index 651ea95f..964b8c10 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) {
@@ -24,10 +24,13 @@ int getentropy(void *buffer, size_t len)
 		}
 		pos += ret;
 		len -= ret;
-		ret = 0;
 	}
 
 	pthread_setcancelstate(cs, 0);
 
-	return ret;
+	if (len) {
+		errno = EIO;
+		return -1;
+	}
+	return 0;
 }
-- 
2.35.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [musl] [PATCH v3] getentropy: fail if buffer not completely filled
  2022-04-09 22:58     ` [musl] [PATCH v2] " Jason A. Donenfeld
@ 2022-04-09 22:58       ` Jason A. Donenfeld
  2022-04-10 15:30       ` [musl] [PATCH v2] " Rich Felker
  1 sibling, 0 replies; 6+ messages in thread
From: Jason A. Donenfeld @ 2022-04-09 22:58 UTC (permalink / raw)
  To: Rich Felker, musl; +Cc: Jason A. Donenfeld

The man page for getentropy says that it either completely succeeds or
completely fails for values < 256, so we can simplify this scenario by
omitting the loop. As a safeguard, we still return EIO if it returns
short, but otherwise we pass the error on through to the caller.
---
Changes v2->v3:
- v3 gets rid of the loop entirely.

 src/misc/getentropy.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/misc/getentropy.c b/src/misc/getentropy.c
index 651ea95f..e8cb4d02 100644
--- a/src/misc/getentropy.c
+++ b/src/misc/getentropy.c
@@ -6,8 +6,8 @@
 
 int getentropy(void *buffer, size_t len)
 {
-	int cs, ret = 0;
-	char *pos = buffer;
+	ssize_t ret;
+	int cs;
 
 	if (len > 256) {
 		errno = EIO;
@@ -15,19 +15,13 @@ int getentropy(void *buffer, size_t len)
 	}
 
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
-
-	while (len) {
-		ret = getrandom(pos, len, 0);
-		if (ret < 0) {
-			if (errno == EINTR) continue;
-			else break;
-		}
-		pos += ret;
-		len -= ret;
-		ret = 0;
-	}
-
+	ret = getrandom(buffer, len, 0);
 	pthread_setcancelstate(cs, 0);
 
-	return ret;
+	if (ret != len) {
+		if (ret >= 0)
+			errno = EIO;
+		return -1;
+	}
+	return 0;
 }
-- 
2.35.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] [PATCH v2] getentropy: fail if buffer not completely filled
  2022-04-09 22:58     ` [musl] [PATCH v2] " Jason A. Donenfeld
  2022-04-09 22:58       ` [musl] [PATCH v3] " Jason A. Donenfeld
@ 2022-04-10 15:30       ` Rich Felker
  1 sibling, 0 replies; 6+ messages in thread
From: Rich Felker @ 2022-04-10 15:30 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: musl

On Sun, Apr 10, 2022 at 12:58:49AM +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.

It does not return success if it breaks out of the loop early. In that
case ret is -1 and it returns -1 (return ret;).

The loop is necessary by my understanding of the function not because
of the possibility of short reads (which shouldn't be able to happen)
but because EINTR can happen while blocking before the urandom pool is
ready. In theory we could probably rip out the non-EINTR part of the
logic (partial reads) but if it's already there and working it seems
like it's not harming anything and serves as hardening against the
kernel doing something stupid.

Is there a remaining problem you're trying to solve?

Rich

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-04-10 15:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-09  0:10 [musl] [PATCH] getentropy: fail if buffer not completely filled Jason A. Donenfeld
2022-04-09 13:18 ` Rich Felker
2022-04-09 22:50   ` Jason A. Donenfeld
2022-04-09 22:58     ` [musl] [PATCH v2] " Jason A. Donenfeld
2022-04-09 22:58       ` [musl] [PATCH v3] " Jason A. Donenfeld
2022-04-10 15:30       ` [musl] [PATCH v2] " Rich Felker

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).