Re: svn commit: r307148 - in head/lib/libc: gen stdlib

[Ed Maste]

On 13 October 2016 at 02:05, Bruce Evans  wrote:
> On Wed, 12 Oct 2016, Ed Maste wrote:
>
> The comment starts by being just wrong:
>
> 1. "The" sysctl is not used here.  Instead, a wrapper arc4_sysctl() is used.
>The wrapper handles some but not all errors.

Fixed in my WIP tree.

> 2. The sysctl can and does fail.  It fails on:
>- all old kernels mixed with new userlands

We don't support new userlands on very old kernels, and I think there
are many other things in libc that will fail on kernels old enough to
lack kern.arandom.

>- with new kernels, at boot time, before the random device is seeded.

If that is indeed still possible it's a bug we need to fix before 12.0.

> 3. The sysctl can, or at least used to, return short reads with nonzero
>counts.

That was addressed in markm's 2015 random work, I think. Presumably
random() was silently broken for the rand_type != TYPE_0 case prior to
that.

>The documentation for this is well hidden, but the
>arc4_sysctl() wrapper exists to support short reads, or perhaps just
>the special case of short reads of 0, which it handles poorly by
>possibly spinning forever.

I suspect we can just remove the arc4_sysctl wrapper too.

> Then the excuse is wrong.  abort() never makes sense in library functions.

arc4random must not return without good quality random data. The other
option would be for it to loop indefinitely.

> Here it gives very confusing errors for the delicate boot-time fandago
> case.

The "delicate boot-time fandango case" was a bug.

> Style bugs:
> - sentence breaks are 2 spaces in KNF, and all old code in this file follows
>   that rule.

Fixed in my WIP tree.

> - 'abort' is not marked up

Fixed in my WIP tree.

> This is even more broken, since it doesn't have the wrapper.

This and the other issues predate my changes; I'll take a look at the
history soon.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r307148 - in head/lib/libc: gen stdlib

[Bruce Evans]

On Wed, 12 Oct 2016, Ed Maste wrote:


Log:
 Add comment on use of abort() in libc

 Suggested by:  jonathan (in review D8133)


It is almost easier to fix the bugs than add the comment.


Modified: head/lib/libc/gen/arc4random.c
==
--- head/lib/libc/gen/arc4random.c  Wed Oct 12 13:51:41 2016
(r307147)
+++ head/lib/libc/gen/arc4random.c  Wed Oct 12 13:56:14 2016
(r307148)
@@ -144,8 +144,15 @@ arc4_stir(void)
arc4_init();
rs_initialized = 1;
}
-   if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE)
-   abort(); /* Random sysctl cannot fail. */
+   if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE) {
+   /*
+* The sysctl cannot fail. If it does fail on some FreeBSD
+* derivative or after some future change, just abort so that
+* the problem will be found and fixed. abort is not normally
+* suitable for a library but makes sense here.
+*/
+   abort();
+   }


The comment starts by being just wrong:

1. "The" sysctl is not used here.  Instead, a wrapper arc4_sysctl() is used.
   The wrapper handles some but not all errors.
2. The sysctl can and does fail.  It fails on:
   - all old kernels mixed with new userlands
   - with new kernels, at boot time, before the random device is seeded.

 I couldn't get this to happen in practice, but it used to be a large
 problem ("Dance fandago on keyboard to unblock"), and source code
 still has large warnings about it.  Apparently des's preseeding based
 on device attach times fixes it for me.  I use the new kernels with
3. The sysctl can, or at least used to, return short reads with nonzero
   counts.  The documentation for this is well hidden, but the
   arc4_sysctl() wrapper exists to support short reads, or perhaps just
   the special case of short reads of 0, which it handles poorly by
   possibly spinning forever.

   I couldn't get this to happen either.  I think it takes O_NONBLOCK.
   With interrupts but without O_NONBLOCK, I just got nice denial of
   service attacks with simple dd tests like "dd bs=200m  silently truncates to 256 bytes.

Then the excuse is wrong.  abort() never makes sense in library functions.
Here it gives very confusing errors for the delicate boot-time fandago
case.

Style bugs:
- sentence breaks are 2 spaces in KNF, and all old code in this file follows
  that rule.
- 'abort' is not marked up


arc4_addrandom(rdat, KEYSIZE);


Modified: head/lib/libc/stdlib/random.c
==
--- head/lib/libc/stdlib/random.c   Wed Oct 12 13:51:41 2016
(r307147)
+++ head/lib/libc/stdlib/random.c   Wed Oct 12 13:56:14 2016
(r307148)
@@ -279,8 +279,15 @@ srandomdev(void)

mib[0] = CTL_KERN;
mib[1] = KERN_ARND;
-   if (sysctl(mib, 2, state, , NULL, 0) == -1 || len != expected)
+   if (sysctl(mib, 2, state, , NULL, 0) == -1 || len != expected) {
+   /*
+* The sysctl cannot fail. If it does fail on some FreeBSD


This is even more broken, since it doesn't have the wrapper.

All the old versions using [_]read() mishandled short reads, but this was
less broken when there was a fallback.  For some reason the sysctl wrapper
in arc4random.c alone handled short reads.


+* derivative or after some future change, just abort so that
+* the problem will be found and fixed. abort is not normally
+* suitable for a library but makes sense here.
+*/
abort();
+   }

if (rand_type != TYPE_0) {
fptr = [rand_sep];


There are also gratuitous namespace differences and bugs.  arc4random()
is less standard than random(), so it can use sysctl() without being
technically broken by namespace pollution.  But it uses __sysctl(),
and has style bugs to declare this.  OTOH, random() is now standard
in POSIX, so it has a reason to use __sysctl(), but it just uses sysctl().
Both should use _sysctlbyname(), and this should be declared in
namespace.h.  Old code used _read() instead of read(), but that seems
to be nonsense since read() is more standard than random().

The old code with the fallback to read() was not wrong (except for the
missing short read heandling).  It gave portability to old kernels.  If
the sysctl were documented, then I think its documentation would say
that it acts like a special case of read (without O_NONBLOCK or EINTR
handling), perhaps on a slightly different device than /dev/random,
except for some technical differences for error reporting.  So the read()
is just as secure as the sysctl.  But with no documentation, we can't
tell what the differences are.

Bruce
___
svn-src-all@freebsd.org mailing list

svn commit: r307148 - in head/lib/libc: gen stdlib

[Ed Maste]

Author: emaste
Date: Wed Oct 12 13:56:14 2016
New Revision: 307148
URL: https://svnweb.freebsd.org/changeset/base/307148

Log:
  Add comment on use of abort() in libc
  
  Suggested by: jonathan (in review D8133)

Modified:
  head/lib/libc/gen/arc4random.c
  head/lib/libc/stdlib/random.c

Modified: head/lib/libc/gen/arc4random.c
==
--- head/lib/libc/gen/arc4random.c  Wed Oct 12 13:51:41 2016
(r307147)
+++ head/lib/libc/gen/arc4random.c  Wed Oct 12 13:56:14 2016
(r307148)
@@ -144,8 +144,15 @@ arc4_stir(void)
arc4_init();
rs_initialized = 1;
}
-   if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE)
-   abort(); /* Random sysctl cannot fail. */
+   if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE) {
+   /*
+* The sysctl cannot fail. If it does fail on some FreeBSD
+* derivative or after some future change, just abort so that
+* the problem will be found and fixed. abort is not normally
+* suitable for a library but makes sense here.
+*/
+   abort();
+   }
 
arc4_addrandom(rdat, KEYSIZE);
 

Modified: head/lib/libc/stdlib/random.c
==
--- head/lib/libc/stdlib/random.c   Wed Oct 12 13:51:41 2016
(r307147)
+++ head/lib/libc/stdlib/random.c   Wed Oct 12 13:56:14 2016
(r307148)
@@ -279,8 +279,15 @@ srandomdev(void)
 
mib[0] = CTL_KERN;
mib[1] = KERN_ARND;
-   if (sysctl(mib, 2, state, , NULL, 0) == -1 || len != expected)
+   if (sysctl(mib, 2, state, , NULL, 0) == -1 || len != expected) {
+   /*
+* The sysctl cannot fail. If it does fail on some FreeBSD
+* derivative or after some future change, just abort so that
+* the problem will be found and fixed. abort is not normally
+* suitable for a library but makes sense here.
+*/
abort();
+   }
 
if (rand_type != TYPE_0) {
fptr = [rand_sep];
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"