Date: Wed, 5 Oct 2016 20:27:14 -0400 From: "Philip M. Gollucci" <pgollucci@p6m7g8.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Alan Cox <alc@freebsd.org>, src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r306713 - in user/alc/PQ_LAUNDRY: lib/libc/stdlib lib/msun/ld80 lib/msun/src sys/cam sys/vm Message-ID: <CACM2dAaU33=noBBc2TXRJR3BAHe0MjyF1gkh10k=dRbkF=-RHg@mail.gmail.com> In-Reply-To: <20161006101333.N1163@besplex.bde.org> References: <201610051803.u95I3Hq1040052@repo.freebsd.org> <CACM2dAa0XrUm4zBaPnSQGrHY8%2B2RLYU5PKC%2BtgDTPrP0_r52XQ@mail.gmail.com> <20161006101333.N1163@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Duh, I missed that sysctl writes to it. My fault. You're of course correct. On Wed, Oct 5, 2016 at 8:13 PM, Bruce Evans <brde@optusnet.com.au> wrote: > On Wed, 5 Oct 2016, Philip M. Gollucci wrote: > > I know you(Alan) didn't write this, its from the MFH, but how would len != >> expected. >> >> neither are initialized or passed in >> both times its set its expected = len = foo >> > > Er, both are initialized. len is passed as an input/output parameter to > sysctl(). sysctl() can return a short count. This is now detected by > recording the expected value in 'expected' and mishandled. Previously, > the error wasn't even detected. > > On Wed, Oct 5, 2016 at 2:03 PM, Alan Cox <alc@freebsd.org> wrote: >> >> Modified: user/alc/PQ_LAUNDRY/lib/libc/stdlib/random.c >>> ============================================================ >>> ================== >>> --- user/alc/PQ_LAUNDRY/lib/libc/stdlib/random.c Wed Oct 5 >>> 17:32:06 2016 (r306712) >>> +++ user/alc/PQ_LAUNDRY/lib/libc/stdlib/random.c Wed Oct 5 >>> 18:03:17 2016 (r306713) >>> @@ -270,16 +270,17 @@ void >>> srandomdev(void) >>> { >>> int mib[2]; >>> - size_t len; >>> + size_t expected, len; >>> >>> if (rand_type == TYPE_0) >>> - len = sizeof(state[0]); >>> + expected = len = sizeof(state[0]); >>> else >>> - len = rand_deg * sizeof(state[0]); >>> + expected = len = rand_deg * sizeof(state[0]); >>> >>> mib[0] = CTL_KERN; >>> mib[1] = KERN_ARND; >>> - sysctl(mib, 2, state, &len, NULL, 0); >>> + if (sysctl(mib, 2, state, &len, NULL, 0) == -1 || len != >>> expected) >>> + abort(); >>> >>> if (rand_type != TYPE_0) { >>> fptr = &state[rand_sep]; >>> >> > I don't like this set of changes. > > Originally, srandomdev() read from /dev/random. read() can also return a > short count. This was detected and mishandled, but not as badly as now. > The non-error of a short count was treated as an error, and both were > handled by falling back to the weak method of using gettimeofday(). > > This was broken by switching to the sysctl and not even checking for > errors from the sysctl. The sysctl can and does fail, mainly when a > new userland is used with an old kernel. > > This commit restores some error checking. > > The implementation uses the style bugs of using the numbered sysctl > KERN_ARND. This sysctl shouldn't exist by number, and isn't documented > by number. It is too new to need a number or benefit from the careful > documentation of old numbered sysctls. It is only documented by name > (kern.arandom), only in a wrong place (random(4)), and a naive reader > would expect from reading this man page that /dev/random is still the > primary interface. Its behaviour of returning a short count might be > expected from the device's behaviour, but is not really documented for > the sysctl. It is only documented that the sysctl will not return > random bytes unless the random device is seeded. The return value for > this case is undocumented (is it 0 or -1? Source code seems to say > that it is 0). Anyway, errors seems to be possible, so abort() is not > acceptable handling. > > This was handled much better in arc4random() and is still handled better > there despite recent regressions: > - the sysctl is wrapped by a function that retries for short counts. > However, the sysctl usage has even larger style bugs -- the sysctl is > by number, and sysctl() is named __sysctl() > - error handling was not missing. Perhaps the retry loop can spin forever > with a short count of 0, but if the sysctl fails then there was a > fallback > first to reading /dev/random and then to the weak gettimeofday() method. > > Now there is no fallback, and the difference is just the retry loop. > > The first step in the removed fallback in arc4random() was not weaker > like the log message says. It is to the primary method according to > the man page. According to the man page, reading /dev/random blocks > until the RNG is seeded for the first time. Blocking for the sysctl > is undocumented. I think the sysctl returns a short count of 0, and > we treat this as an error and abort(), so applications running early > in the boot crash instead of having the traditional behaviour of hanging. > > It is unclear if short counts of nonzero can actually occur. I think > they could easily occur in older implementations where /dev/random > blocked for more cases than before the initial seeding. I think they > should still be possible. Even if there is no need to block, it is > useful to be able to kill a read() with a preposterously large count. > If interrupts are allowed at all, then after one the return value must > be either -1 or a short count. That is another reason why abort() in > a library is bad error handling. It turns most signals into SIGABRT. > > Bruce > -- --------------------------------------------------------------------------------- 4096R/D21D2752 <http://pgp.mit.edu/pks/lookup?op=get&search=0xF699A450D21D2752> ECDF B597 B54B 7F92 753E E0EA F699 A450 D21D 2752 Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354 Member, Apache Software Foundation Committer, FreeBSD Foundation Consultant, P6M7G8 Inc. Director Cloud Technology, Capital One What doesn't kill us can only make us stronger; Except it almost kills you.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACM2dAaU33=noBBc2TXRJR3BAHe0MjyF1gkh10k=dRbkF=-RHg>