From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 14464 invoked from network); 9 Apr 2022 13:18:34 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 9 Apr 2022 13:18:34 -0000 Received: (qmail 12028 invoked by uid 550); 9 Apr 2022 13:18:31 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 11991 invoked from network); 9 Apr 2022 13:18:31 -0000 Date: Sat, 9 Apr 2022 09:18:17 -0400 From: Rich Felker To: "Jason A. Donenfeld" Cc: musl@lists.openwall.com Message-ID: <20220409131816.GJ7074@brightrain.aerifal.cx> References: <20220409001047.234283-1-Jason@zx2c4.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220409001047.234283-1-Jason@zx2c4.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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