Hi Stephan, On Tue, 14 Jul 2020 at 16:34, Stephan Mueller <smueller@xxxxxxxxxx> wrote: > > Am Dienstag, 14. Juli 2020, 17:23:05 CEST schrieb Elena Petrova: > > Hi Elena, > > > Hi Stephan, > > > > On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <smueller@xxxxxxxxxx> wrote: > > > Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova: > > > > > > Hi Elena, > > > > > > > This patch extends the userspace RNG interface to make it usable for > > > > CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY > > > > option to the setsockopt interface for specifying the entropy, and using > > > > sendmsg syscall for specifying the additional data. > > > > > > > > See libkcapi patch [1] to test the added functionality. The libkcapi > > > > patch is not intended for merging into libkcapi as is: it is only a > > > > quick plug to manually verify that the extended AF_ALG RNG interface > > > > generates the expected output on DRBG800-90A CAVS inputs. > > > > > > As I am responsible for developing such CAVS/ACVP harness as well, I > > > played > > > with the idea of going through AF_ALG. I discarded it because I do not see > > > the benefit why we should add an interface solely for the purpose of > > > testing. Further, it is a potentially dangerous one because the created > > > instance of the DRBG is "seeded" from data provided by the caller. > > > > > > Thus, I do not see the benefit from adding that extension, widening a user > > > space interface solely for the purpose of CAVS testing. I would not see > > > any > > > other benefit we have with this extension. In particular, this interface > > > would then be always there. What I could live with is an interface that > > > can be enabled at compile time for those who want it. > > > > Thanks for reviewing this patch. I understand your concern about the > > erroneous use of the entropy input by non-testing applications. This was an > > approach that I had discussed with Ard. I should have included you, my > > apologies. I'll post v2 with the CAVS testing stuff hidden under CONFIG_ > > option with appropriate help text. > > > > With regards to the usefulness, let me elaborate. This effort of extending > > the drbg interface is driven by Android needs for having the kernel crypto > > certified. I started from having an out-of-tree chrdev driver for Google > > Pixel kernels that was exposing the required crypto functionality, and it > > wasn't ideal in the following ways: > > * it primarily consisted of copypasted code from testmgr.c > > * it was hard for me to keep the code up to date because I'm not aware of > > ongoing changes to crypto > > * it is hard for other people and/or organisations to re-use it, hense a > > lot of duplicated effort is going into CAVS: you have a private driver, I > > have mine, Jaya from HP <jayalakshmi.bhat@xxxxxx>, who's been asking > > linux-crypto a few CAVS questions, has to develop theirs... > > > > In general Android is trying to eliminate out-of-tree code. CAVS testing > > functionality in particular needs to be upstream because we are switching > > all Android devices to using a Generic Kernel Image (GKI) > > [https://lwn.net/Articles/771974/] based on the upstream kernel. > > Thank you for the explanation. > > > > > Besides, when you want to do CAVS testing, the following ciphers are still > > > not testable and thus this patch would only be a partial solution to get > > > the testing covered: > > > > > > - AES KW (you cannot get the final IV out of the kernel - I played with > > > the > > > idea to return the IV through AF_ALG, but discarded it because of the > > > concern above) > > > > > > - OFB/CFB MCT testing (you need the IV from the last round - same issue as > > > for AES KW) > > > > > > - RSA > > > > > > - DH > > > > > > - ECDH > > > > For Android certification purposes, we only need to test the modules which > > are actually being used. In our case it's what af_alg already exposes plus > > DRBG. If, say, HP would need RSA, they could submit their own patch. > > > > As for exposing bits of the internal state for some algorithms, I hope > > guarding the testing functionality with a CONFIG_ option would solve the > > security part of the problem. > > Yes, for all other users. > > But if you are planning to enable this option for all Android devices across > the board I am not sure here. In this case, wouldn't it make sense to require > capable(CAP_SYS_ADMIN) for the DRBG reset operation just as an additional > precaution? Note, the issue with the reset is that you loose all previous > state (which is good for testing, but bad for security as I guess you agree > :-) ). I believe that for Android, since CAVS is a one-off on demand operation, we can create a separate build with CONFIG_CRYPTO_CAVS_DRBG enabled, which won't go to the users. Thanks for suggesting capabilities check, happy to add `capable(CAP_SYS_ADMIN)` regardless. > > > With these issues, I would assume you are better off creating your own > > > kernel module just like I did that externalizes the crypto API to user > > > space but is only available on your test kernel and will not affect all > > > other users. > > I considered publishing my kernel driver on GitHub, but there appears to be > > a sufficiently large number of users to justify having this functionality > > upstream. > > So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches then? :-D Sure :) > > > > Hope that addresses your concerns. > > > > > Ciao > > > Stephan > > > > Thanks, > > Elena > > > Ciao > Stephan > >
