Re: svn commit: r230230 - head/sys/dev/random
On Mon, Jan 30, 2012 at 11:30:15AM +, Mark Murray wrote: > > Well, I almost forget about my special case: I have personal prohibition > > from @secteam (5 years old already) to commit anything to all RNG areas. > > > > So, the question is: could anyone of you commit some version from this > > thread, please? > > Sure; I'll do it. Please give me your test code/cases. > > > I don't insist of atomics in this sutuation, so you can peek any version > > you like. > > I'll need to clearly see what works. Both works:) Version with atomic cmpsets works 100% correct, but it seems people dislike it just for using atomics. Version without atomics works slightly incorrectly in edge cases, but no harm happens. Worst possible scenario for version without atomics: 1) Several arc4random() fired at once (i.e. concurrently) exact in the moment when random_yarrow_unblock() modifies this variable. 2) Such of them who catch the variable after modification and see that reseed is needed simultaneously put themselves into reseeding chain, because arc4_randomstir() is protected with mutex. There is no harm can be done to this PRNG by reseeding it many times sequentially, just waste of CPU & time. 3) Such of them who miss the modification skips this step, and PRNG will be reseded when any other arc4random() call happens afterwards. The rest of arc4rand() function code is protected by mutex too, so it will stay sequentially-aligned in any case. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes: > On Thu, Jan 26, 2012 at 10:13:41PM +0400, Andrey Chernov wrote: > > On Thu, Jan 26, 2012 at 12:52:43PM -0500, David Schultz wrote: > > > Why complicate things with atomics at all? A race might result in > > > arc4random(9) being seeded multiple times, but that's harmless. > > > > Multiply seeding in line is harmless, just waste of time and resources. > > Other case is one missing seeding when variable is set concurrently with > > its read. I see no complication using atomic. Latest version is even > > shorter than previous ones. > > Well, I almost forget about my special case: I have personal prohibition > from @secteam (5 years old already) to commit anything to all RNG areas. > > So, the question is: could anyone of you commit some version from this > thread, please? Sure; I'll do it. Please give me your test code/cases. > I don't insist of atomics in this sutuation, so you can peek any version > you like. I'll need to clearly see what works. M -- Mark R V Murray Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open) Pi: 132511160 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 10:13:41PM +0400, Andrey Chernov wrote: > On Thu, Jan 26, 2012 at 12:52:43PM -0500, David Schultz wrote: > > Why complicate things with atomics at all? A race might result in > > arc4random(9) being seeded multiple times, but that's harmless. > > Multiply seeding in line is harmless, just waste of time and resources. > Other case is one missing seeding when variable is set concurrently with > its read. I see no complication using atomic. Latest version is even > shorter than previous ones. Well, I almost forget about my special case: I have personal prohibition from @secteam (5 years old already) to commit anything to all RNG areas. So, the question is: could anyone of you commit some version from this thread, please? I don't insist of atomics in this sutuation, so you can peek any version you like. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Sat, Jan 28, 2012 at 06:47:50PM +1100, Bruce Evans wrote:
> > --- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
> > +++ sys/libkern.h 2012-01-28 08:49:19.0 +0400
> > @@ -70,6 +70,11 @@ static __inline int abs(int a) { return
> > static __inline long labs(long a) { return (a < 0 ? -a : a); }
> > static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); }
> >
> > +#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */
> > +#defineARC4_ENTR_HAVE 1 /* Have entropy. */
> > +#defineARC4_ENTR_SEED 2 /* Reseeding. */
> > +extern int arc4rand_iniseed_state;
> > +
> > /* Prototypes for non-quad routines. */
> > struct malloc_type;
> > uint32_t arc4random(void);
>
> ... I will also ask for data and macros to be sorted into the data an
> macro sections and not unsorted in between the first set of inlines
> and the prototypes. (Whether the macros should be all in a macro
> section or near the variables or prototypes that they are for is
> unclear, especially since the current organization for this in this
> file is random.)
>
> This file was last nearly sorted in FreeBSD-2.1. Now its organization
> is partly to put macros and data together with inline functions that
> use them. This gives lots of subsections which are not clearly delimited
> and is not used in the prototype section, where it would split up that
> section too much.
It is unclear for me what exact place(s) you suggest for those
defines/extern now, keeping in mind that I don't want to reorganize old
code at this moment, so the patch should be at the minimum level.
> > About atomic ops: arc4rand calls are protected with mutex and yarrow
> > calls are protected, but they can works concurrently wich each
> > other. So, if we have atomic ops, why not to use them to close one time
> > window too?
>
> Hmm, it's tricky code either way. I use similar cmpsets in sio, and
> also need a loop to synchronize the final state change. But when the
> state is seen to be final, atomic ops are not needed to check that it
> doesn't change (or to see that it is final), since it won't change again.
> This gives a tiny optimization that would be larger here. This is mostly
I can try to eliminate after-done repeating of those cmpsets using plain
static variables in yarrow and arc4rand, but this code should be run under
mutex. For arc4rand at least it means that yet one cmpset (non-repeated,
now under mutext) should be added. Is such complications worth something?
Are cmpsets overkills or require heavy processor resources?
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Sat, 28 Jan 2012, Andrey Chernov wrote:
New verson addressed bde's style things:
Thanks, but ...
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-28 08:49:19.0 +0400
@@ -70,6 +70,11 @@ static __inline int abs(int a) { return
static __inline long labs(long a) { return (a < 0 ? -a : a); }
static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); }
+#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */
+#defineARC4_ENTR_HAVE 1 /* Have entropy. */
+#defineARC4_ENTR_SEED 2 /* Reseeding. */
+extern int arc4rand_iniseed_state;
+
/* Prototypes for non-quad routines. */
struct malloc_type;
uint32_t arc4random(void);
... I will also ask for data and macros to be sorted into the data an
macro sections and not unsorted in between the first set of inlines
and the prototypes. (Whether the macros should be all in a macro
section or near the variables or prototypes that they are for is
unclear, especially since the current organization for this in this
file is random.)
This file was last nearly sorted in FreeBSD-2.1. Now its organization
is partly to put macros and data together with inline functions that
use them. This gives lots of subsections which are not clearly delimited
and is not used in the prototype section, where it would split up that
section too much.
From your previous reply (non-style part):
> > +volatile int arc4rand_iniseed_state = ARC4_ENTR_NONE;
> > +
>
> I don't see what all the volatile qualifiers and atomic ops are for.
> The volatiles certainly have no effect, since this variable is only
> accessed by atomic ops which only access variables as volatile whether
> you want this or not. See das's reply for more about the atomic ops.
> Here I think they just close a window that would be open just once for
> each for a few instructions while booting.
I will remove volatile.
About atomic ops: arc4rand calls are protected with mutex and yarrow
calls are protected, but they can works concurrently wich each
other. So, if we have atomic ops, why not to use them to close one time
window too?
Hmm, it's tricky code either way. I use similar cmpsets in sio, and
also need a loop to synchronize the final state change. But when the
state is seen to be final, atomic ops are not needed to check that it
doesn't change (or to see that it is final), since it won't change again.
This gives a tiny optimization that would be larger here. This is mostly
overkill, since sio is^Wwas normally present at boot time. New-bus and
most drivers don't have complications like this, but might need them more
for loadable drivers. It is very unclear that new-bus's Giant locking is
enough for anything (unless the whole system also uses Giant locking).
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
New verson addressed bde's style things:
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-28 08:49:19.0 +0400
@@ -70,6 +70,11 @@ static __inline int abs(int a) { return
static __inline long labs(long a) { return (a < 0 ? -a : a); }
static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); }
+#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */
+#defineARC4_ENTR_HAVE 1 /* Have entropy. */
+#defineARC4_ENTR_SEED 2 /* Reseeding. */
+extern int arc4rand_iniseed_state;
+
/* Prototypes for non-quad routines. */
struct malloc_type;
uint32_t arc4random(void);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-28 08:48:22.0 +0400
@@ -366,6 +366,8 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ (void)atomic_cmpset_int(&arc4rand_iniseed_state, ARC4_ENTR_NONE,
+ ARC4_ENTR_HAVE);
}
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-28 08:51:12.0 +0400
@@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+int arc4rand_iniseed_state = ARC4_ENTR_NONE;
+
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -130,7 +132,8 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (atomic_cmpset_int(&arc4rand_iniseed_state, ARC4_ENTR_HAVE,
+ ARC4_ENTR_SEED) || reseed ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Fri, Jan 27, 2012 at 08:34:35PM +1100, Bruce Evans wrote: > On Thu, 26 Jan 2012, Andrey Chernov wrote: > > >> On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote: > >>> Atomics don't operate on enums. You'll need to make it an int and just > >>> use > >>> #define's for the 3 states. > > This restores some style bugs and keeps old ones. About old bugs - I don't want this patch to be intrusive and touch style of old things. It can be committed as separate patch. > > /* Prototypes for non-quad routines. */ > > struct malloc_type; > > +#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */ > > +#defineARC4_ENTR_HAVE 1 /* Have entropy. */ > > +#defineARC4_ENTR_SEED 2 /* Reseeding. */ > > +extern volatile int arc4rand_iniseed_state; > > I see no prototypes here. Will be moved before the comment. > > uint32_t arc4random(void); > > void arc4rand(void *ptr, u_int len, int reseed); > > I see a prototype with style bugs here (names for parameters, which isn't > bug for bug compatible with the other prototypes). I don't wan't to touch old things for now. > > int bcmp(const void *, const void *, size_t); > > --- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300 > > +++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400 > > @@ -366,6 +366,8 @@ random_yarrow_unblock(void) > > selwakeuppri(&random_systat.rsel, PUSER); > > wakeup(&random_systat); > > } > > + (void)atomic_cmpset_int(&arc4rand_iniseed_state, > > + ARC4_ENTR_NONE, ARC4_ENTR_HAVE); > > } > > This is gnu style. Continuation indentation is 4 spaces in KNF. Will be 4-space indented. > > +volatile int arc4rand_iniseed_state = ARC4_ENTR_NONE; > > + > > I don't see what all the volatile qualifiers and atomic ops are for. > The volatiles certainly have no effect, since this variable is only > accessed by atomic ops which only access variables as volatile whether > you want this or not. See das's reply for more about the atomic ops. > Here I think they just close a window that would be open just once for > each for a few instructions while booting. I will remove volatile. About atomic ops: arc4rand calls are protected with mutex and yarrow calls are protected, but they can works concurrently wich each other. So, if we have atomic ops, why not to use them to close one time window too? > > + if (atomic_cmpset_int(&arc4rand_iniseed_state, > > + ARC4_ENTR_HAVE, ARC4_ENTR_SEED) || > > Gnu style indentation. Excessively early line splitting. Will be fixed. > Old code uses KNF style (except for excessive line splitting and excessive > parentheses) in the same statement. Old code is not in question at this moment. Thanx. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, 26 Jan 2012, Andrey Chernov wrote:
On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote:
Atomics don't operate on enums. You'll need to make it an int and just use
#define's for the 3 states.
This restores some style bugs and keeps old ones.
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-26 21:40:21.0 +0400
@@ -72,6 +72,10 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */
+#defineARC4_ENTR_HAVE 1 /* Have entropy. */
+#defineARC4_ENTR_SEED 2 /* Reseeding. */
+extern volatile int arc4rand_iniseed_state;
I see no prototypes here.
uint32_t arc4random(void);
void arc4rand(void *ptr, u_int len, int reseed);
I see a prototype with style bugs here (names for parameters, which isn't
bug for bug compatible with the other prototypes).
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400
@@ -366,6 +366,8 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ (void)atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_NONE, ARC4_ENTR_HAVE);
}
This is gnu style. Continuation indentation is 4 spaces in KNF.
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-26 21:40:47.0 +0400
@@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#define ARC4_RESEED_SECONDS 300
#define ARC4_KEYBYTES (256 / 8)
+volatile int arc4rand_iniseed_state = ARC4_ENTR_NONE;
+
I don't see what all the volatile qualifiers and atomic ops are for.
The volatiles certainly have no effect, since this variable is only
accessed by atomic ops which only access variables as volatile whether
you want this or not. See das's reply for more about the atomic ops.
Here I think they just close a window that would be open just once for
each for a few instructions while booting.
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -130,7 +132,9 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_HAVE, ARC4_ENTR_SEED) ||
Gnu style indentation. Excessively early line splitting.
+ reseed ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
Old code uses KNF style (except for excessive line splitting and excessive
parentheses) in the same statement.
arc4_randomstir();
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
David Schultz writes: > > Although current version with current kernel flags works, I forget > > it is implementation defined in general and not always equal to > > sizeof(int), f.e. with gcc --short-enums. I'll remade it with > > #defines, thanx again. > > Why complicate things with atomics at all? A race might result in > arc4random(9) being seeded multiple times, but that's harmless. > > The race that worries me is that consumers that call arc4random() > before it is properly seeded will get predictable numbers. To fix > that robustly, we'd either have to move arc4random() into the random > module (tricky given all the places where it's used), or make the > random module a mandatory part of the kernel. There is a VERY old problem here, and it is of the chicken vs egg variety. The random device won't unlock until it has enough entropy, and there are things in the kernel that appear to need "random enough" numbers earlier than that. It ought to be possible to unlock the CSPRNG by harvesting entropy earlier, but this is hard work. Very hard work. Unless you have entropy-generating hardware. In the meanwhile, you are left with other alternatives; reseed arc4random() with some early entropy; try to move entropy consumers to later in the boot sequence; try to not depend on entropy early on, and correct for this later (where necessary). There are loads of other alternatives. What you can't do is unlock before you have enough entropy, and that won't happen until the running kernel has had some time to accumulate this. I am in favour of treating arc4random() as a PRNG with (say) the date/time, the TSC register and other easy-to-get data as the first seed. After that its treated as a CSPRNG. This requires that early consumers know the limitations and adapt to them. The notion of making arc4random() block would be silly; the point of the thing is that it always returns numbers, and after the random device seeds, it should always be good. > OpenSSL addresses the issue by providing two APIs: RAND_bytes() > requires a good entropy source and produces cryptographically strong > pseudorandomness. RAND_pseudo_bytes() produces "good" (but not > necessarily unpredictable) randomness, even in the absence of an > entropy source. Applications call one interface or the other, > depending on whether they require cryptographic- quality randomness. You are treading on graves here :-). An ancient bikeshed is buried in the vicinty. M -- Mark R V Murray Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open) Pi: 132511160 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 12:52:43PM -0500, David Schultz wrote: > Why complicate things with atomics at all? A race might result in > arc4random(9) being seeded multiple times, but that's harmless. Multiply seeding in line is harmless, just waste of time and resources. Other case is one missing seeding when variable is set concurrently with its read. I see no complication using atomic. Latest version is even shorter than previous ones. > The race that worries me is that consumers that call arc4random() > before it is properly seeded will get predictable numbers. To fix > that robustly, we'd either have to move arc4random() into the > random module (tricky given all the places where it's used), or > make the random module a mandatory part of the kernel. I already vote for second option for various reasons. The problem still is more complicated, because arc4random() used very early in the net code and can't wait until yarrow harvests enough entropy, especially for net boot cases. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 06:34:13PM +0100, Stefan Farfeleder wrote: > > The type of an enumerator actually is `int', so it should be fine. Please ignore that, I misread the diff. Stefan ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote:
> On Thursday, January 26, 2012 10:56:27 am Andrey Chernov wrote:
> > > On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote:
> > > > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE,
> ARC4_ENTER_HAVE);
> > > > break;
> >
> > Updated version (I hope, final):
> >
> > --- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
> > +++ sys/libkern.h 2012-01-26 19:38:06.0 +0400
> > @@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
> >
> > /* Prototypes for non-quad routines. */
> > struct malloc_type;
> > +enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
> > +extern volatile enum arc4_is arc4rand_iniseed_state;
>
> Atomics don't operate on enums. You'll need to make it an int and just use
> #define's for the 3 states.
The type of an enumerator actually is `int', so it should be fine.
>
> > uint32_t arc4random(void);
> > voidarc4rand(void *ptr, u_int len, int reseed);
> > int bcmp(const void *, const void *, size_t);
> > --- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
> > +++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400
> > @@ -366,6 +366,8 @@ random_yarrow_unblock(void)
> > selwakeuppri(&random_systat.rsel, PUSER);
> > wakeup(&random_systat);
> > }
> > + (void)atomic_cmpset_int(&arc4rand_iniseed_state,
> > + ARC4_ENTR_NONE, ARC4_ENTR_HAVE);
> > }
Stefan
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012, Andrey Chernov wrote:
> On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote:
> > On Thursday, January 26, 2012 10:56:27 am Andrey Chernov wrote:
> > > > On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote:
> > > > > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE,
> > ARC4_ENTER_HAVE);
> > > > > break;
> > >
> > > Updated version (I hope, final):
> > >
> > > --- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
> > > +++ sys/libkern.h 2012-01-26 19:38:06.0 +0400
> > > @@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
> > >
> > > /* Prototypes for non-quad routines. */
> > > struct malloc_type;
> > > +enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
> > > +extern volatile enum arc4_is arc4rand_iniseed_state;
> >
> > Atomics don't operate on enums. You'll need to make it an int and just use
> > #define's for the 3 states.
>
> Although current version with current kernel flags works, I forget it is
> implementation defined in general and not always equal to sizeof(int),
> f.e. with gcc --short-enums. I'll remade it with #defines, thanx again.
Why complicate things with atomics at all? A race might result in
arc4random(9) being seeded multiple times, but that's harmless.
The race that worries me is that consumers that call arc4random()
before it is properly seeded will get predictable numbers. To fix
that robustly, we'd either have to move arc4random() into the
random module (tricky given all the places where it's used), or
make the random module a mandatory part of the kernel.
OpenSSL addresses the issue by providing two APIs: RAND_bytes()
requires a good entropy source and produces cryptographically
strong pseudorandomness. RAND_pseudo_bytes() produces "good"
(but not necessarily unpredictable) randomness, even in the
absence of an entropy source. Applications call one interface
or the other, depending on whether they require cryptographic-
quality randomness.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
> On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote:
> > Atomics don't operate on enums. You'll need to make it an int and just use
> > #define's for the 3 states.
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-26 21:40:21.0 +0400
@@ -72,6 +72,10 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+#defineARC4_ENTR_NONE 0 /* Don't have entropy yet. */
+#defineARC4_ENTR_HAVE 1 /* Have entropy. */
+#defineARC4_ENTR_SEED 2 /* Reseeding. */
+extern volatile int arc4rand_iniseed_state;
uint32_t arc4random(void);
voidarc4rand(void *ptr, u_int len, int reseed);
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400
@@ -366,6 +366,8 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ (void)atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_NONE, ARC4_ENTR_HAVE);
}
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-26 21:40:47.0 +0400
@@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+volatile int arc4rand_iniseed_state = ARC4_ENTR_NONE;
+
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -130,7 +132,9 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_HAVE, ARC4_ENTR_SEED) ||
+ reseed ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 11:32:38AM -0500, John Baldwin wrote:
> On Thursday, January 26, 2012 10:56:27 am Andrey Chernov wrote:
> > > On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote:
> > > > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE,
> ARC4_ENTER_HAVE);
> > > > break;
> >
> > Updated version (I hope, final):
> >
> > --- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
> > +++ sys/libkern.h 2012-01-26 19:38:06.0 +0400
> > @@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
> >
> > /* Prototypes for non-quad routines. */
> > struct malloc_type;
> > +enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
> > +extern volatile enum arc4_is arc4rand_iniseed_state;
>
> Atomics don't operate on enums. You'll need to make it an int and just use
> #define's for the 3 states.
Although current version with current kernel flags works, I forget it is
implementation defined in general and not always equal to sizeof(int),
f.e. with gcc --short-enums. I'll remade it with #defines, thanx again.
> I think the rest of this is fine.
>
> --
> John Baldwin
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thursday, January 26, 2012 10:56:27 am Andrey Chernov wrote:
> > On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote:
> > > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE,
ARC4_ENTER_HAVE);
> > > break;
>
> Updated version (I hope, final):
>
> --- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
> +++ sys/libkern.h 2012-01-26 19:38:06.0 +0400
> @@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
>
> /* Prototypes for non-quad routines. */
> struct malloc_type;
> +enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
> +extern volatile enum arc4_is arc4rand_iniseed_state;
Atomics don't operate on enums. You'll need to make it an int and just use
#define's for the 3 states.
> uint32_t arc4random(void);
> void arc4rand(void *ptr, u_int len, int reseed);
> int bcmp(const void *, const void *, size_t);
> --- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
> +++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400
> @@ -366,6 +366,8 @@ random_yarrow_unblock(void)
> selwakeuppri(&random_systat.rsel, PUSER);
> wakeup(&random_systat);
> }
> + (void)atomic_cmpset_int(&arc4rand_iniseed_state,
> + ARC4_ENTR_NONE, ARC4_ENTR_HAVE);
> }
>
> static int
> --- libkern/arc4random.c.old 2008-08-08 01:51:09.0 +0400
> +++ libkern/arc4random.c 2012-01-26 19:43:31.0 +0400
> @@ -24,6 +24,7 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
> #define ARC4_RESEED_SECONDS 300
> #define ARC4_KEYBYTES (256 / 8)
>
> +volatile enum arc4_is arc4rand_iniseed_state = ARC4_ENTR_NONE;
> static u_int8_t arc4_i, arc4_j;
> static int arc4_numruns = 0;
> static u_int8_t arc4_sbox[256];
> @@ -130,7 +131,9 @@ arc4rand(void *ptr, u_int len, int resee
> struct timeval tv;
>
> getmicrouptime(&tv);
> - if (reseed ||
> + if (atomic_cmpset_int(&arc4rand_iniseed_state,
> + ARC4_ENTR_HAVE, ARC4_ENTR_DONE) ||
> +reseed ||
> (arc4_numruns > ARC4_RESEED_BYTES) ||
> (tv.tv_sec > arc4_t_reseed))
> arc4_randomstir();
I think the rest of this is fine.
--
John Baldwin
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
> On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote:
> > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE,
> > ARC4_ENTER_HAVE);
> > break;
Updated version (I hope, final):
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-26 19:38:06.0 +0400
@@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
+extern volatile enum arc4_is arc4rand_iniseed_state;
uint32_t arc4random(void);
voidarc4rand(void *ptr, u_int len, int reseed);
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-26 19:35:12.0 +0400
@@ -366,6 +366,8 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ (void)atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_NONE, ARC4_ENTR_HAVE);
}
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-26 19:43:31.0 +0400
@@ -24,6 +24,7 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+volatile enum arc4_is arc4rand_iniseed_state = ARC4_ENTR_NONE;
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -130,7 +131,9 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (atomic_cmpset_int(&arc4rand_iniseed_state,
+ ARC4_ENTR_HAVE, ARC4_ENTR_DONE) ||
+ reseed ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 08:39:07AM -0500, John Baldwin wrote: > What is the purpose of the atomics? Doing atomic_load/atomic_store > is just as racy as if you had not used atomics at all. Thanx for a hint. Protecting comparison itself isn't essential as protecting variable consitency because random_yarrow_unblock() which triggers operation isn't called concurrently, but arc4rand() is. I consider your suggested way a bit later. Yes, it seems acq/rel memory barriers are unneded, > Should you be > using atomic_cmpset instead, e.g.: > > case ARC4_ENTER_HAVE: > /* XXX: What does it mean for this to fail? */ It means "Request to have enough entropy, but already have or even already reseed". > atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE, > ARC4_ENTER_HAVE); > break; -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Wednesday, January 25, 2012 10:39:50 pm Andrey Chernov wrote: > On Thu, Jan 26, 2012 at 07:03:05AM +0400, Andrey Chernov wrote: > > On Wed, Jan 25, 2012 at 07:16:41PM +, Mark Murray wrote: > > > I thought you were going to do this as a function? It would be > > > slightly neater to do it that way. > > > > > > Looks good! Are you sure this needs no locking or volatile > > > variables? > > > > Now with function, volatile, atomic and even enum: > > Sorry. Reading of state variable should be atomical too. Fixed version: What is the purpose of the atomics? Doing atomic_load/atomic_store is just as racy as if you had not used atomics at all. Should you be using atomic_cmpset instead, e.g.: case ARC4_ENTER_HAVE: /* XXX: What does it mean for this to fail? */ atomic_cmpset_int(&iniseed_state, ARC4_ENTER_NONE, ARC4_ENTER_HAVE); break; -- John Baldwin ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 26, 2012 at 07:03:05AM +0400, Andrey Chernov wrote:
> On Wed, Jan 25, 2012 at 07:16:41PM +, Mark Murray wrote:
> > I thought you were going to do this as a function? It would be
> > slightly neater to do it that way.
> >
> > Looks good! Are you sure this needs no locking or volatile
> > variables?
>
> Now with function, volatile, atomic and even enum:
Sorry. Reading of state variable should be atomical too. Fixed version:
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-26 06:01:51.0 +0400
@@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
+void arc4rand_iniseed_state(enum arc4_is state);
uint32_t arc4random(void);
voidarc4rand(void *ptr, u_int len, int reseed);
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-26 06:04:05.0 +0400
@@ -366,6 +366,7 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ arc4rand_iniseed_state(ARC4_ENTR_HAVE);
}
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-26 07:27:06.0 +0400
@@ -24,6 +24,7 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+static volatile enum arc4_is iniseed_state = ARC4_ENTR_NONE;
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -74,6 +75,7 @@ arc4_randomstir (void)
/* Reset for next reseed cycle. */
arc4_t_reseed = tv_now.tv_sec + ARC4_RESEED_SECONDS;
arc4_numruns = 0;
+ arc4rand_iniseed_state(ARC4_ENTR_DONE);
/*
* Throw away the first N words of output, as suggested in the
@@ -103,6 +105,24 @@ arc4_init(void)
SYSINIT(arc4_init, SI_SUB_LOCK, SI_ORDER_ANY, arc4_init, NULL);
+void
+arc4rand_iniseed_state(enum arc4_is state)
+{
+ switch (state) {
+ case ARC4_ENTR_NONE:
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ case ARC4_ENTR_HAVE:
+ if (atomic_load_acq_int(&iniseed_state) ==
ARC4_ENTR_NONE)
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ case ARC4_ENTR_DONE:
+ if (atomic_load_acq_int(&iniseed_state) ==
ARC4_ENTR_HAVE)
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ }
+}
+
/*
* Generate a random byte.
*/
@@ -130,7 +150,7 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (reseed || atomic_load_acq_int(&iniseed_state) == ARC4_ENTR_HAVE ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Wed, Jan 25, 2012 at 07:16:41PM +, Mark Murray wrote:
> I thought you were going to do this as a function? It would be
> slightly neater to do it that way.
>
> Looks good! Are you sure this needs no locking or volatile
> variables?
Now with function, volatile, atomic and even enum:
--- sys/libkern.h.old 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-26 06:01:51.0 +0400
@@ -72,6 +72,8 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+enum arc4_is { ARC4_ENTR_NONE, ARC4_ENTR_HAVE, ARC4_ENTR_DONE };
+void arc4rand_iniseed_state(enum arc4_is state);
uint32_t arc4random(void);
voidarc4rand(void *ptr, u_int len, int reseed);
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.old 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-26 06:04:05.0 +0400
@@ -366,6 +366,7 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ arc4rand_iniseed_state(ARC4_ENTR_HAVE);
}
static int
--- libkern/arc4random.c.old2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-26 06:01:07.0 +0400
@@ -24,6 +24,7 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+static volatile enum arc4_is iniseed_state = ARC4_ENTR_NONE;
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -74,6 +75,7 @@ arc4_randomstir (void)
/* Reset for next reseed cycle. */
arc4_t_reseed = tv_now.tv_sec + ARC4_RESEED_SECONDS;
arc4_numruns = 0;
+ arc4rand_iniseed_state(ARC4_ENTR_DONE);
/*
* Throw away the first N words of output, as suggested in the
@@ -103,6 +105,24 @@ arc4_init(void)
SYSINIT(arc4_init, SI_SUB_LOCK, SI_ORDER_ANY, arc4_init, NULL);
+void
+arc4rand_iniseed_state(enum arc4_is state)
+{
+ switch (state) {
+ case ARC4_ENTR_NONE:
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ case ARC4_ENTR_HAVE:
+ if (iniseed_state == ARC4_ENTR_NONE)
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ case ARC4_ENTR_DONE:
+ if (iniseed_state == ARC4_ENTR_HAVE)
+ atomic_store_rel_int(&iniseed_state, state);
+ break;
+ }
+}
+
/*
* Generate a random byte.
*/
@@ -130,7 +150,7 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (reseed || iniseed_state == ARC4_ENTR_HAVE ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes:
> On Sun, Jan 22, 2012 at 09:43:02PM +, Mark Murray wrote:
> > > Thanx for review! I'll send final version to this thread a bit
> > > later when I'll find more free time.
>
> Final, unless something else noticed.
Cool. NOTE: I am only eyeballing this, not testing it.
> --- sys/libkern.h.bak 2012-01-16 07:15:12.0 +0400
> +++ sys/libkern.h 2012-01-25 17:31:49.0 +0400
> @@ -72,6 +72,7 @@ static __inline quad_t qabs(quad_t a) {
>
> /* Prototypes for non-quad routines. */
> struct malloc_type;
> +extern int arc4rand_iniseed_state;
> uint32_t arc4random(void);
> void arc4rand(void *ptr, u_int len, int reseed);
> int bcmp(const void *, const void *, size_t);
Fine.
> --- dev/random/randomdev_soft.c.bak 2011-03-02 01:42:19.0 +0300
> +++ dev/random/randomdev_soft.c 2012-01-25 17:28:19.0 +0400
> @@ -366,6 +366,8 @@ random_yarrow_unblock(void)
> selwakeuppri(&random_systat.rsel, PUSER);
> wakeup(&random_systat);
> }
> + if (arc4rand_iniseed_state == 0)
> + arc4rand_iniseed_state = 1;
> }
>
> static int
I thought you were going to do this as a function? It would be
slightly neater to do it that way.
> --- libkern/arc4random.c.bak 2008-08-08 01:51:09.0 +0400
> +++ libkern/arc4random.c 2012-01-25 17:30:30.0 +0400
> @@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
> #define ARC4_RESEED_SECONDS 300
> #define ARC4_KEYBYTES (256 / 8)
>
> +int arc4rand_iniseed_state = 0;
> +
> static u_int8_t arc4_i, arc4_j;
> static int arc4_numruns = 0;
> static u_int8_t arc4_sbox[256];
> @@ -74,6 +76,8 @@ arc4_randomstir (void)
> /* Reset for next reseed cycle. */
> arc4_t_reseed = tv_now.tv_sec + ARC4_RESEED_SECONDS;
> arc4_numruns = 0;
> + if (arc4rand_iniseed_state == 1)
> + arc4rand_iniseed_state = -1;
>
> /*
>* Throw away the first N words of output, as suggested in the
> @@ -130,7 +134,7 @@ arc4rand(void *ptr, u_int len, int resee
> struct timeval tv;
>
> getmicrouptime(&tv);
> - if (reseed ||
> + if (reseed || arc4rand_iniseed_state == 1 ||
> (arc4_numruns > ARC4_RESEED_BYTES) ||
> (tv.tv_sec > arc4_t_reseed))
> arc4_randomstir();
Looks good! Are you sure this needs no locking or volatile
variables?
M
--
Mark R V Murray
Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open)
Pi: 132511160
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Sun, Jan 22, 2012 at 09:43:02PM +, Mark Murray wrote:
> > Thanx for review! I'll send final version to this thread a bit
> > later when I'll find more free time.
Final, unless something else noticed.
--- sys/libkern.h.bak 2012-01-16 07:15:12.0 +0400
+++ sys/libkern.h 2012-01-25 17:31:49.0 +0400
@@ -72,6 +72,7 @@ static __inline quad_t qabs(quad_t a) {
/* Prototypes for non-quad routines. */
struct malloc_type;
+extern int arc4rand_iniseed_state;
uint32_t arc4random(void);
voidarc4rand(void *ptr, u_int len, int reseed);
int bcmp(const void *, const void *, size_t);
--- dev/random/randomdev_soft.c.bak 2011-03-02 01:42:19.0 +0300
+++ dev/random/randomdev_soft.c 2012-01-25 17:28:19.0 +0400
@@ -366,6 +366,8 @@ random_yarrow_unblock(void)
selwakeuppri(&random_systat.rsel, PUSER);
wakeup(&random_systat);
}
+ if (arc4rand_iniseed_state == 0)
+ arc4rand_iniseed_state = 1;
}
static int
--- libkern/arc4random.c.bak2008-08-08 01:51:09.0 +0400
+++ libkern/arc4random.c2012-01-25 17:30:30.0 +0400
@@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+int arc4rand_iniseed_state = 0;
+
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -74,6 +76,8 @@ arc4_randomstir (void)
/* Reset for next reseed cycle. */
arc4_t_reseed = tv_now.tv_sec + ARC4_RESEED_SECONDS;
arc4_numruns = 0;
+ if (arc4rand_iniseed_state == 1)
+ arc4rand_iniseed_state = -1;
/*
* Throw away the first N words of output, as suggested in the
@@ -130,7 +134,7 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (reseed || arc4rand_iniseed_state == 1 ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
(tv.tv_sec > arc4_t_reseed))
arc4_randomstir();
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes: > > Should be in a header file, nad _possibly_ should be volatile. If it > > works without being volatile, then OK. > > It was preliminary patch just to confirm/deny my understanding of your > idea. Ah, OK - in which case you got the idea correctly! > I'll put it into header. Cool. > In the final version I also plan to move that lines > + if (arc4rand_iniseed_state == 1) > + arc4rand_iniseed_state = -1; > into arc4_randomstir() where they will be protected with mutex lock, so > volatile will be not needed. It will be more logical, because other > reseeding conditions are resetted there too. Great. > > The rest is OK. I've not tested it, so this is not a review, simply an > > "OK" :-) > > Thanx for review! I'll send final version to this thread a bit > later when I'll find more free time. No problem. M -- Mark R V Murray Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open) Pi: 132511160 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Sun, Jan 22, 2012 at 04:59:55PM +, Mark Murray wrote:
> Andrey Chernov writes:
> > > The usual way round this is with a flag. Set a static, volatile
> > > flag, defaulting "off", and set it to "on" when the seeding has
> > > happened. Then arc4random() can do the right thing, depending on
> > > this flag.
> >
> > Ok, what about this version, is it right? libkern/arc4rand.c is not a
> > module but always present in the kernel, so "arc4rand_iniseed_state"
> > will be always accessible.
> >
> > --- dev/random/randomdev_soft.c.old 2011-09-26 07:35:48.0 +0400
> > +++ dev/random/randomdev_soft.c 2012-01-21 01:41:37.0 +0400
> > @@ -55,6 +55,8 @@ __FBSDID("$FreeBSD: src/sys/dev/random/r
> >
> > #define RANDOM_FIFO_MAX256 /* How many events to queue up */
> >
> > +extern int arc4rand_iniseed_state;
> > +
>
> Should be in a header file, nad _possibly_ should be volatile. If it
> works without being volatile, then OK.
It was preliminary patch just to confirm/deny my understanding of your
idea.
I'll put it into header.
In the final version I also plan to move that lines
+ if (arc4rand_iniseed_state == 1)
+ arc4rand_iniseed_state = -1;
into arc4_randomstir() where they will be protected with mutex lock, so
volatile will be not needed. It will be more logical, because other
reseeding conditions are resetted there too.
> The rest is OK. I've not tested it, so this is not a review, simply an
> "OK" :-)
Thanx for review! I'll send final version to this thread a bit
later when I'll find more free time.
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes:
> > The usual way round this is with a flag. Set a static, volatile
> > flag, defaulting "off", and set it to "on" when the seeding has
> > happened. Then arc4random() can do the right thing, depending on
> > this flag.
>
> Ok, what about this version, is it right? libkern/arc4rand.c is not a
> module but always present in the kernel, so "arc4rand_iniseed_state"
> will be always accessible.
>
> --- dev/random/randomdev_soft.c.old 2011-09-26 07:35:48.0 +0400
> +++ dev/random/randomdev_soft.c 2012-01-21 01:41:37.0 +0400
> @@ -55,6 +55,8 @@ __FBSDID("$FreeBSD: src/sys/dev/random/r
>
> #define RANDOM_FIFO_MAX 256 /* How many events to queue up */
>
> +extern int arc4rand_iniseed_state;
> +
Should be in a header file, nad _possibly_ should be volatile. If it
works without being volatile, then OK.
The rest is OK. I've not tested it, so this is not a review, simply an
"OK" :-)
M
--
Mark R V Murray
Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open)
Pi: 132511160
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Fri, Jan 20, 2012 at 03:12:53PM +, Mark Murray wrote:
> Andrey Chernov writes:
> > > Look at the function random_yarrow_unblock(). Thats where yopu want to
> > > be doing this. This function is where the random device is unblocked
> > > once safely seeded.
> >
> > Thanx for your hint, but I fear one moment using random_yarrow_unblock().
> > It is called under mtx_lock(&random_reseed_mtx) in reseed().
> > And when arc4rand() seeding is called, it uses read_random(), so I see
> > possible deadlock can happens.
>
> The usual way round this is with a flag. Set a static, volatile flag,
> defaulting
> "off", and set it to "on" when the seeding has happened. Then arc4random() can
> do the right thing, depending on this flag.
Ok, what about this version, is it right? libkern/arc4rand.c is not
a module but always present in the kernel, so "arc4rand_iniseed_state" will be
always accessible.
--- dev/random/randomdev_soft.c.old 2011-09-26 07:35:48.0 +0400
+++ dev/random/randomdev_soft.c 2012-01-21 01:41:37.0 +0400
@@ -55,6 +55,8 @@ __FBSDID("$FreeBSD: src/sys/dev/random/r
#define RANDOM_FIFO_MAX256 /* How many events to queue up */
+extern int arc4rand_iniseed_state;
+
static void random_kthread(void *);
static void
random_harvest_internal(u_int64_t, const void *, u_int,
@@ -361,6 +363,8 @@ random_yarrow_write(void *buf, int count
void
random_yarrow_unblock(void)
{
+ if (arc4rand_iniseed_state == 0)
+ arc4rand_iniseed_state = 1;
if (!random_systat.seeded) {
random_systat.seeded = 1;
selwakeuppri(&random_systat.rsel, PUSER);
--- libkern/arc4random.c.old2011-09-26 07:37:23.0 +0400
+++ libkern/arc4random.c2012-01-21 01:46:53.0 +0400
@@ -24,6 +24,8 @@ __FBSDID("$FreeBSD: src/sys/libkern/arc4
#defineARC4_RESEED_SECONDS 300
#defineARC4_KEYBYTES (256 / 8)
+int arc4rand_iniseed_state = 0;
+
static u_int8_t arc4_i, arc4_j;
static int arc4_numruns = 0;
static u_int8_t arc4_sbox[256];
@@ -130,10 +132,13 @@ arc4rand(void *ptr, u_int len, int resee
struct timeval tv;
getmicrouptime(&tv);
- if (reseed ||
+ if (reseed || arc4rand_iniseed_state == 1 ||
(arc4_numruns > ARC4_RESEED_BYTES) ||
- (tv.tv_sec > arc4_t_reseed))
+ (tv.tv_sec > arc4_t_reseed)) {
+ if (arc4rand_iniseed_state == 1)
+ arc4rand_iniseed_state = -1;
arc4_randomstir();
+ }
mtx_lock(&arc4_mtx);
arc4_numruns += len;
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes: > > Look at the function random_yarrow_unblock(). Thats where yopu want to > > be doing this. This function is where the random device is unblocked > > once safely seeded. > > Thanx for your hint, but I fear one moment using random_yarrow_unblock(). > It is called under mtx_lock(&random_reseed_mtx) in reseed(). > And when arc4rand() seeding is called, it uses read_random(), so I see > possible deadlock can happens. The usual way round this is with a flag. Set a static, volatile flag, defaulting "off", and set it to "on" when the seeding has happened. Then arc4random() can do the right thing, depending on this flag. > In my version arc4rand() seeding happens only when this lock is released, > so no blocking is possible. Sure, but the dependancies created are problematic in their own right. M -- Mark R V Murray Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open) Pi: 132511160 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Thu, Jan 19, 2012 at 07:52:30PM +, Mark Murray wrote: > Andrey Chernov writes: > > On Mon, Jan 16, 2012 at 08:18:10PM +, David Schultz wrote: > > > Author: das > > > Date: Mon Jan 16 20:18:10 2012 > > > New Revision: 230230 > > > URL: http://svn.freebsd.org/changeset/base/230230 > > > > > > Log: > > > Generate a warning if the kernel's arc4random() is seeded with bogus > > > entropy. > > > > While you are here, could you review/commit my patch to fix bad 31bit > > arc4rand() seeding, please? > > > > --- yarrow.c.bak2011-09-26 07:35:48.0 +0400 > > +++ yarrow.c2012-01-18 10:13:47.0 +0400 > > This is the wrong place for this; it may achieve the desired result, but > the file is where the Yarrow algorithm is implepeneted; ARC4 reseeds are > not a part of that, which makes this proposal a layering violation at > best, and an unwarranted dependancy at worst. > > Look at the function random_yarrow_unblock(). Thats where yopu want to > be doing this. This function is where the random device is unblocked > once safely seeded. Thanx for your hint, but I fear one moment using random_yarrow_unblock(). It is called under mtx_lock(&random_reseed_mtx) in reseed(). And when arc4rand() seeding is called, it uses read_random(), so I see possible deadlock can happens. In my version arc4rand() seeding happens only when this lock is released, so no blocking is possible. But perhaps I oversight something, correct me if I am wrong, please. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
Andrey Chernov writes: > On Mon, Jan 16, 2012 at 08:18:10PM +, David Schultz wrote: > > Author: das > > Date: Mon Jan 16 20:18:10 2012 > > New Revision: 230230 > > URL: http://svn.freebsd.org/changeset/base/230230 > > > > Log: > > Generate a warning if the kernel's arc4random() is seeded with bogus > > entropy. > > While you are here, could you review/commit my patch to fix bad 31bit > arc4rand() seeding, please? > > --- yarrow.c.bak 2011-09-26 07:35:48.0 +0400 > +++ yarrow.c 2012-01-18 10:13:47.0 +0400 This is the wrong place for this; it may achieve the desired result, but the file is where the Yarrow algorithm is implepeneted; ARC4 reseeds are not a part of that, which makes this proposal a layering violation at best, and an unwarranted dependancy at worst. Look at the function random_yarrow_unblock(). Thats where yopu want to be doing this. This function is where the random device is unblocked once safely seeded. M -- Mark R V Murray Cert APS(Open) Dip Phys(Open) BSc Open(Open) BSc(Hons)(Open) Pi: 132511160 ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Wed, Jan 18, 2012 at 12:54:40PM -0500, David Schultz wrote: > It appears to reseed arc4random's state exactly once, at whatever > unpredictable time devrandom decides to reseed itself. Are you As fast as possible, immediatelly when we have enough good entropy. > trying to fix the problems that arise if random.ko is loaded too > late in the boot process? There is only _initial_ seeding security problem with arc4rand() and not only when random.ko is not loaded, but when it is loaded too and don't harvest enough entropy yet. All late stages don't have security problem because arc4rand() periodically reseeds itself from yarrow when ARC4_RESEED_SECONDS is expired. About random.ko loading itself, this is separate question and I already express opinion to make random.ko not optional but required kernel module. -- http://ache.vniz.net/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Wed, Jan 18, 2012, Andrey Chernov wrote:
> On Mon, Jan 16, 2012 at 08:18:10PM +, David Schultz wrote:
> > Author: das
> > Date: Mon Jan 16 20:18:10 2012
> > New Revision: 230230
> > URL: http://svn.freebsd.org/changeset/base/230230
> >
> > Log:
> > Generate a warning if the kernel's arc4random() is seeded with bogus
> > entropy.
>
> While you are here, could you review/commit my patch to fix bad 31bit
> arc4rand() seeding, please?
>
> --- yarrow.c.bak 2011-09-26 07:35:48.0 +0400
> +++ yarrow.c 2012-01-18 10:13:47.0 +0400
> @@ -59,6 +59,8 @@ static void reseed(u_int);
> /* The reseed thread mutex */
> struct mtx random_reseed_mtx;
>
> +static arc4rand_seeded = 0;
> +
> /* Process a single stochastic event off the harvest queue */
> void
> random_process_event(struct harvest *event)
> @@ -261,6 +263,11 @@ reseed(u_int fastslow)
>
> /* Release the reseed mutex */
> mtx_unlock(&random_reseed_mtx);
> +
> + if (!arc4rand_seeded) {
> + arc4rand_seeded = 1;
> + arc4rand(NULL, 0, 1);
> + }
> }
>
> /* Internal function to return processed entropy from the PRNG */
It appears to reseed arc4random's state exactly once, at whatever
unpredictable time devrandom decides to reseed itself. Are you
trying to fix the problems that arise if random.ko is loaded too
late in the boot process?
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230230 - head/sys/dev/random
On Mon, Jan 16, 2012 at 08:18:10PM +, David Schultz wrote:
> Author: das
> Date: Mon Jan 16 20:18:10 2012
> New Revision: 230230
> URL: http://svn.freebsd.org/changeset/base/230230
>
> Log:
> Generate a warning if the kernel's arc4random() is seeded with bogus
> entropy.
While you are here, could you review/commit my patch to fix bad 31bit
arc4rand() seeding, please?
--- yarrow.c.bak2011-09-26 07:35:48.0 +0400
+++ yarrow.c2012-01-18 10:13:47.0 +0400
@@ -59,6 +59,8 @@ static void reseed(u_int);
/* The reseed thread mutex */
struct mtx random_reseed_mtx;
+static arc4rand_seeded = 0;
+
/* Process a single stochastic event off the harvest queue */
void
random_process_event(struct harvest *event)
@@ -261,6 +263,11 @@ reseed(u_int fastslow)
/* Release the reseed mutex */
mtx_unlock(&random_reseed_mtx);
+
+ if (!arc4rand_seeded) {
+ arc4rand_seeded = 1;
+ arc4rand(NULL, 0, 1);
+ }
}
/* Internal function to return processed entropy from the PRNG */
--
http://ache.vniz.net/
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r230230 - head/sys/dev/random
Author: das
Date: Mon Jan 16 20:18:10 2012
New Revision: 230230
URL: http://svn.freebsd.org/changeset/base/230230
Log:
Generate a warning if the kernel's arc4random() is seeded with bogus entropy.
Modified:
head/sys/dev/random/harvest.c
Modified: head/sys/dev/random/harvest.c
==
--- head/sys/dev/random/harvest.c Mon Jan 16 20:17:51 2012
(r230229)
+++ head/sys/dev/random/harvest.c Mon Jan 16 20:18:10 2012
(r230230)
@@ -37,6 +37,7 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
+#include
#include
#include
@@ -48,6 +49,7 @@ static int read_random_phony(void *, int
/* Structure holding the desired entropy sources */
struct harvest_select harvest = { 1, 1, 1, 0 };
+static int warned = 0;
/* hold the address of the routine which is actually called if
* the randomdev is loaded
@@ -71,6 +73,7 @@ random_yarrow_deinit_harvester(void)
{
reap_func = NULL;
read_func = read_random_phony;
+ warned = 0;
}
/* Entropy harvesting routine. This is supposed to be fast; do
@@ -108,6 +111,11 @@ read_random_phony(void *buf, int count)
u_long randval;
int size, i;
+ if (!warned) {
+ log(LOG_WARNING, "random device not loaded; using insecure
entropy\n");
+ warned = 1;
+ }
+
/* srandom() is called in kern/init_main.c:proc0_post() */
/* Fill buf[] with random(9) output */
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
