Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220725225728.824128-1-Jason@zx2c4.com> <20220725232810.843433-1-Jason@zx2c4.com>
 <CAMdZqKH=9mDhoW_gpL-pUEQAGuN=orc1doudyAuHdoPc7O53RQ@mail.gmail.com> <Yt/EySjdJjYW/EcB@zx2c4.com>
In-Reply-To: <Yt/EySjdJjYW/EcB@zx2c4.com>
From:   Mark Harris <mark.hsj@gmail.com>
Date:   Tue, 26 Jul 2022 09:51:03 -0700
Message-ID: <CAMdZqKGzhajnb5ejypnPFanJ2E=4Pk_96x8FAAShttvnrRenfQ@mail.gmail.com>
Subject: Re: [PATCH v2] arc4random: simplify design for better safety
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     libc-alpha@sourceware.org, Florian Weimer <fweimer@redhat.com>,
        linux-crypto@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Jason A. Donenfeld wrote:
> On Mon, Jul 25, 2022 at 06:10:06PM -0700, Mark Harris wrote:
> > Jason A. Donenfeld wrote:
> > > +      l = __getrandom_nocancel (p, n, 0);
> > > +      if (l > 0)
> > > +       {
> > > +         if ((size_t) l == n)
> > > +           return; /* Done reading, success. */
> > > +         p = (uint8_t *) p + l;
> > > +         n -= l;
> > > +         continue; /* Interrupted by a signal; keep going. */
> > > +       }
> > > +      else if (l == 0)
> > > +       arc4random_getrandom_failure (); /* Weird, should never happen. */
> > > +      else if (errno == ENOSYS)
> > > +       {
> > > +         have_getrandom = false;
> > > +         break; /* No syscall, so fallback to /dev/urandom. */
> > > +       }
> > > +      arc4random_getrandom_failure (); /* Unknown error, should never happen. */
> >
> > Isn't EINTR also possible?  Aborting in that case does not seem reasonable.
>
> Not in current kernels, where it always returns at least PAGE_SIZE bytes
> before checking for pending signals. In older kernels, if there was a
> signal pending at the top, it would do no work and return -ERESTARTSYS,
> which I believe should then get restarted by glibc's syscaller? I might
> be wrong about how restarts work though, so if you know better, please
> let me know. TEMP_FAILURE_RETRY relies on errno, so that's not what we
> want. I guess I can just add a case for it.
>
> > Also the __getrandom_nocancel function does not set errno on Linux; it
> > just returns INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags).
> > So unless that is changed, it doesn't look like this ENOSYS check will
> > detect old Linux kernels.
>
> Thanks. It looks like INTERNAL_SYSCALL_CALL just returns the errno as-is
> as a return value, right? I'll adjust the code to account for that.

Yes INTERNAL_SYSCALL_CALL just returns the negated errno value that it
gets from the Linux kernel, but only on Linux does
__getrandom_nocancel use that.  The Hurd and generic implementations
set errno on error.  Previously the only call to this function did not
care about the specific error value so it didn't matter.  Since you
are now using the error value in generic code, __getrandom_nocancel
should be changed on Linux to set errno like most other _nocancel
calls, and then it should go back to checking errno here.

And as Adhemerval mentioned, you only added a Linux implementation of
__ppoll_infinity_nocancel, but are calling it from generic code.

Also, by the way your patches cc'd directly to me get quarantined
because DKIM signature verification failed.  The non-patch messages
pass DKIM and are fine.


 - Mark