Krishna, Thanks for the code review! You are always very thorough.
Mark Krishna Yenduri wrote: > Mark Powers wrote: > >> ... >> The webrev is >> http://cr.grommit.com/~mcpowers/acceleration/webrev.opensolaris >> > > I am sorry for the delay with this review. I have reviewed all > pkcs11_kernel changes. This concludes my code review. > > -Krishna > > > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelAttributeUtil.c > > KY-0 line 853 Do we need CKK_X9_42_DH also? I guess it depends on whether > there are consumers for it. I don't know. If we do, that work will be done under a different RFE. > > KY-1 line 953 This can be just if > (pslot->sl_func_list.fl_object_create && > since fl_object_create is a boolean. ACCEPT > > KY-2 line 1304,1307 No need for the checks because we do them earlier ACCEPT > > KY-3 line 1777,1780 Same comment as above. ACCEPT > > > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelKeys.c > > KY-4 lines 91,93 This can be made conditional on > if (i != j) [ ACCEPT > > KY-5 lines 295,302 > lines 745,747,762,764 > lines 1023,1025,1040,1042 > lines 1302,1304,1319,1321 > lines 2231,2232,2240 > > It looks these could result in double free if we get rv != CRYPTO_SUCCESS REJECT The function free_attributes() sets the count of attributes to 0 after they have been freed. That's why we pass a pointer to the count. > > KY-6 line 319 > lines 796,797 > lines 1082,1083 > lines 1353,1354 > line 2256 > > These are unneeded. ACCEPT > > KY-7 lines 391,1456,2334 Consider adding a check to see if the token > supports key generation by value. We might as well > fail early. > Not sure what you mean here. The token is advertising key generation mechanisms and doesn't support object creation. That's a pretty good hint that the token supports key generation by value. The only way I can know for sure is to try the ioctl that generates the key. It requires a fair amount of code to set up for the ioctl, so I don't see how I can make the check any earlier. > > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelUtil.c > > KY-8 lines 654,663 Missing pthread_mutex_unlock() ACCEPT I also noticed I was failing to free memory for some of the error returns. > > KY-9 line 712 Consider removing this line and > include lines 719-721. One less "goto" this way :-). ACCEPT > > > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelGlobal.h > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelObject.h > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelObjectUtil.c > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelSign.c > usr/src/lib/pkcs11/pkcs11_kernel/common/kernelVerify.c > > Look fine > >
