Trying again, as Krishna had already answered some of this. Apologies for the half-finished email.
Dina wrote: > > 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. This was already answered. The ifdefs/macros for the mutexes will be removed, but the mutexes will remain. The point was that the pthread library does not incur unnecessary overhead in single-threaded apps. > >> 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. >> > This was answered already. My response, "ok". > >> 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()? > Since KY-4 was addressed already, my comment is no longer relevant. >> 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. > This issue is the one that needs a second look. > > 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. >> > Ok. Thanks for the feedback Krishna. D. > _______________________________________________ > crypto-discuss mailing list > crypto-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crypto-discuss
