Krishna Yenduri wrote: > Dina wrote: >> ... >> >> 6722460 finish moving /dev/random and /dev/urandom seeding and usage >> to libcryptoutil >> 6666204 meta slot opens and closes /dev/urandom needlessly for every read >> >> The webrev is at http://cr.opensolaris.org/~dinak/random-hg >> > > My review comments are below. > > -Krishna > > ------------------------------------------------------------------------------------------- > > > > usr/src/lib/libcryptoutil/common/cryptoutil.h > > KY-1 - lines 144-145 > Highly recommend renaming these to something like > DEV_RANDOM and DEV_URANDOM. It is more clear and > gets you away from defining "AVG_QUALITY". People already know > about /dev/random and /dev/urandom. >
As an aside, see the note at <ONNV>/usr/src/lib/pkcs11/libpkcs11/common/metaRand.c: 51 * NOTES: 52 * 1) /dev/urandom vs. /dev/random... Unfortunately P11 does not allow app 53 * to request a "quality", so we'll just assume urandom is good enough. 54 * Concerned apps can pull hardcore randomness from specific places they 55 * trust (eg by checking for CKF_HW?).. The functions as implemented were intended to make it easier to add support for other sources of random numbers besides /dev/random and /dev/urandom. An application may want to register a custom random generator down the road. That said, I can rename them to SOURCE_DEV_RANDOM and SOURCE_DEV_RANDOM and also rename rquality_t to rsource_t. It still leaves the door open to specify other sources and even assigning/re-assigning a quality value to a given source of random numbers. > KY-2 - lines 151-152 > Consider renaming these to pkcs11_get_random > (or pkcs11_generate_random) and > pkcs11_get_nzero_random. This makes them consistent > with the other routines above. > Ok. > > usr/src/lib/libcryptoutil/common/random.c > > KY-3 T3 lines 34-39 > No need for _REENTRANT madness now that libpthread is folded > in to libc in S10 (See PSARC 2002/217). It should be the > default and lines 44-49 can go away. > > Recommend removing the RAND_LOCK macros also. > You mean PSARC 2002/117. I don't understand your comment. The reason for the code you flagged here is so that all the mutex locking/unlocking are no-ops if random.c was not compiled with the -mt compiler option. Do you still feel they should be removed? Then there is no locking and the caller needs to implement it. > > KY-4 T2 lines 198 and 216, 223 and 241 > This results in a performance problem for the multi-thread > case since the global lock is held and released for each call. > > See the current meta_SeedRandom() for the solution. > See > KY-5 T2 lines 214 and 215, 239 and 240 > This results in a performance problem since you are > opening and closing /dev/urandom needlessly for every seed. > There are applications that make this call frequently. > Ok, I can move the close()/=-1 to another function that is to be used to explicitly close the seed fd. For those applications that seed the random generator frequently, what is the approximate proportion of calls to seed_random()/get_random()? > > KY-6 T5 lines 203 and 213, 228 and 238 > Remove the leftover comments. > Will be done as part of KY-4/KY-5 clean up. > > KY-7 T3 lines 253-254, 264 > Comments need updating. > Ok. > KY-8 T2 line 265 > Same issue as KY-4. You are holding and releasing a lock > for each call to this routine by calling pkcs11_open_random. > Ok. > > usr/src/lib/pkcs11/libpkcs11/common/metaGeneral.c > > KY-9 T3 line 198 > Need to close the open fd's (if any) from /dev/random > and /dev/urandom. This is needed even if they are closed > in pkcs11_softtoken since there can be a configuration where > softtoken slot is disabled. Once KY-5 is addressed, the fd > to seed /dev/urandom needs to be closed here. > > Also, there is a change in behavior here that needs to be called > out in comments here - > > With this change, libpkcs11 and pkcs11_softtoken share the > same global fd. This can be a problem if there were ever > a case where pkcs11_softtoken is finalized and closes the > fd while libpkcs11 is still used (say using pkcs11_kernel slots). > If KY-5 were not done, then this would not be a problem for the random seed fd, at least. True, libpkcs11 and pkcs11_softtoken share one fd for /dev/random and /dev/urandom. However if one closes the fd, the other one will re-open the fd when it needs to get more random data (due to the implicit open). It looks like in that situation, where one closes the fd and the other one opens it or vice versa, there is the possibility of a race. Holding the lock longer, which I think you recommended against, helps. But it doesn't eliminate the window between the open and the read. The implementation could be taken further with a SHARED/EXCLUSIVE flag that taken -- implement a SHARED/EXCLUSIVE flag that always opens one sharable random rd, and fcntl(DUPFD) if the caller wants exclusive access to a random fd? > > usr/src/lib/pkcs11/pkcs11_softtoken/common/softGeneral.c > > KY-10 T3 line 328 > Once KY-5 is addressed, the fd to seed /dev/urandom needs > to be closed here. >
