I updated my webrev with review comments from Dan and Ferenc. The updated webrev is at:
http://cr.opensolaris.org/~haimay/CR6703956-v1/ Please reply by Friday 10/24/08. Thanks, Hai-May Hai-May Chao wrote: > Dan, > > Thanks for your review comments. My responses are inline. > > Dan Anderson wrote: > >> FILE usr/src/uts/common/crypto/api/kcf_random.c >> >> 580 #define RND_CPU_PAD (RND_CPU_PAD_SIZE - \ >> 581 (sizeof (kmutex_t) + 3*sizeof (uint8_t *) + sizeof >> (HMAC_KEYSCHED) + \ >> 582 sizeof (uint64_t) + 3*sizeof (uint32_t) + sizeof >> (rnd_stats_t))) + \ >> 583 15*sizeof (uint32_t) >> >> DEA-1. This won't work in computing the correct padding size. Field >> rnd_stats_t has 64-bit integers and are aligned on 0mod8 byte boundaries on >> SPARC or x86/64-bit and, on x86/32-bit, 0mod4 byte boundaries. >> >> But from reading the comments, it seems like the padding size doesn't have >> to be exact. It appears that if the padding is greater than that necessary, >> the purpose is accomplished here (that is, exceed the cache-line size). >> >> Otherwise, a way to avoid padding is to order the 64-bit structure elements >> first, followed by 32-bit elements, and so forth. >> >> > > Agreed. Took a general solution that we change the code to use two > structures > suggested by Ferenc. > > #define RND_CPU_CACHE_SIZE 64 > #define RND_CPU_PAD_SIZE RND_CPU_CACHE_SIZE*6 > #define RND_CPU_PAD (RND_CPU_PAD_SIZE - \ > sizeof(rndmag_t)) > > typedef struct rndmag_s > { > kmutex_t rm_lock; > uint8_t *rm_buffer; /* Start of buffer */ > uint8_t *rm_eptr; /* End of buffer */ > uint8_t *rm_rptr; /* Current read pointer */ > HMAC_KEYSCHED rm_ks; /* seed */ > uint64_t rm_counter; /* rotating counter for extracting */ > uint32_t rm_oblocks; /* time to rekey? */ > uint32_t rm_ofuzz; /* Rekey backoff state */ > uint32_t rm_olimit; /* Hard rekey limit */ > rnd_stats_t rm_stats; /* Per-CPU Statistics */ > uint32_t rm_key[SHA1WORDS]; /* FIP XKEY */ > uint32_t rm_seed[SHA1WORDS]; /* seed for rekey */ > uint32_t rm_previous[SHA1WORDS]; /* previous random bytes */ > } rndmag_t; > > typedef struct rndmag_pad_s > { > rndmag_t rm_mag; > uint8_t rm_pad[RND_CPU_PAD]; > } rndmag_pad_t; > > and changed all the usages. > > > >> 702 bcopy((uint8_t *)tempout, (uint8_t *)rmp->rm_previous, >> 703 HASHSIZE); >> 833 bcopy((uint8_t *)rmp->rm_seed, (uint8_t >> *)rmp->rm_previous, >> 834 HMAC_KEYSIZE); >> DEA-2. Casting uint8_t* is not needed for bcopy() as the bcopy prototype >> uses void*. >> The casting may be needed for other functions, but not bcopy(). >> void* has no alignment or type restriction. >> >> >> > > Ok. removed the casting. > > > >> FILE usr/src/uts/common/crypto/io/swrand.c >> 258 bcopy((uint8_t *)swrand_XKEY, (uint8_t *)previous_bytes, >> HASHSIZE); >> 387 bcopy(digest, (uint8_t *)digest32, HASHSIZE); >> 409 bcopy((uint8_t *)tempout, (uint8_t *)previous_bytes, >> HASHSIZE); >> 410 >> 414 bcopy((uint8_t *)tempout + bytes, >> 415 leftover, leftover_bytes); >> >> See comment above, DEA-2. >> >> > > Ok. removed the casting. > > > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/crypto-discuss/attachments/20081021/4633994d/attachment.html>
