| Message ID | 20181120060217.t4nccaqpwnxkl4tx@gondor.apana.org.au (mailing list archive) |
|---|---|
| Headers | show |
| Series | Exporting existing crypto API code through zinc | expand |
On Tue, 20 Nov 2018 at 07:02, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Mon, Nov 19, 2018 at 01:24:51PM +0800, Herbert Xu wrote: > > > > In response to Martin's patch-set which I merged last week, I think > > here is quick way out for the zinc interface. > > > > Going through the past zinc discussions it would appear that > > everybody is quite happy with the zinc interface per se. The > > most contentious areas are in fact the algorithm implementations > > under zinc, as well as the conversion of the crypto API algorithms > > over to using the zinc interface (e.g., you can no longer access > > specific implementations). > > > > My proposal is to merge the zinc interface as is, but to invert > > how we place the algorithm implementations. IOW the implementations > > should stay where they are now, with in the crypto API. However, > > we will provide direct access to them for zinc without going through > > the crypto API. IOW we'll just export the functions directly. > > > > Here is a proof of concept patch to do it for chacha20-generic. > > It basically replaces patch 3 in the October zinc patch series. > > Actually this patch also exports the arm/x86 chacha20 functions > > too so they are ready for use by zinc. > > > > If everyone is happy with this then we can immediately add the > > zinc interface and expose the existing crypto API algorithms > > through it. This would allow wireguard to be merged right away. > > > > In parallel, the discussions over replacing the implementations > > underneath can carry on without stalling the whole project. > > > > PS This patch is totally untested :) > > Here is an updated version demonstrating how we could access the > accelerated versions of chacha20. It also includes a final patch > to deposit the new zinc version of x86-64 chacha20 into > arch/x86/crypto where it can be used by both the crypto API as well > as zinc. > > The key differences between this and the last zinc patch series: > > 1. The crypto API algorithms remain individually accessible, this > is crucial as these algorithm names are exported to user-space so > changing the names to foo-zinc is not going to work. > Are you saying user space may use names like "ctr-aes-neon" directly rather than "ctr(aes)" for any implementation of the mode? If so, that is highly unfortunate, since it means we'd be breaking user space by wrapping a crypto library function with its own arch specific specializations into a generic crypto API wrapper. Note that I think that using AF_ALG to access software crypto routines (as opposed to accelerators) is rather pointless to begin with, but if it permits that today than we're stuck with it. > 2. The arch-specific algorithm code lives in their own module rather > than in a monolithic module. > This is one of the remaining issues I have with Zinc. However, modulo the above concerns, I think it does make sense to have a separate function API for synchronous software routines below the current crypto API. What I don't want is a separate Zinc kingdom with a different name, different maintainers and going through a different tree, and with its own approach to test vectors, arch specific code, etc etc
Hi Ard: On Tue, Nov 20, 2018 at 11:32:05AM +0100, Ard Biesheuvel wrote: > > > 1. The crypto API algorithms remain individually accessible, this > > is crucial as these algorithm names are exported to user-space so > > changing the names to foo-zinc is not going to work. > > Are you saying user space may use names like "ctr-aes-neon" directly > rather than "ctr(aes)" for any implementation of the mode? Yes. In fact it's used for FIPS certification testing. > If so, that is highly unfortunate, since it means we'd be breaking > user space by wrapping a crypto library function with its own arch > specific specializations into a generic crypto API wrapper. You can never break things by introducing new algorithms. The problem is only when you remove existing ones. > Note that I think that using AF_ALG to access software crypto routines > (as opposed to accelerators) is rather pointless to begin with, but if > it permits that today than we're stuck with it. Sure, nobody sane should be doing it. But when it comes to government certification... :) > > 2. The arch-specific algorithm code lives in their own module rather > > than in a monolithic module. > > This is one of the remaining issues I have with Zinc. However, modulo > the above concerns, I think it does make sense to have a separate > function API for synchronous software routines below the current > crypto API. What I don't want is a separate Zinc kingdom with a > different name, different maintainers and going through a different > tree, and with its own approach to test vectors, arch specific code, > etc etc Even without the issue of replacing chacha20-generic with chacha20-zinc which breaks point 1 above, we simply don't want or need to go through zinc's run-time implementation choice for the crypto API algorithms. They've already paid for the indirect function call so why make them go through yet another run-time branch? If the optics of having the code in crypto is the issue, we could move the actual algorithm code into a third location, perhaps arch/x86/crypto/lib or arch/x86/lib/crypto. Then both zinc and the crypto API can sit on top of that. Cheers,
Hi Herbert, On Tue, Nov 20, 2018 at 7:02 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > Here is an updated version demonstrating how we could access the > accelerated versions of chacha20. It also includes a final patch > to deposit the new zinc version of x86-64 chacha20 into > arch/x86/crypto where it can be used by both the crypto API as well > as zinc. N'ack. As I mentioned in the last email, this really isn't okay, and mostly defeats the purpose of Zinc in the first place, adding complexity in a discombobulated half-baked way. Your proposal seems to be the worst of all worlds. Having talked to lots of people about the design of Zinc at this point to very positive reception, I'm going to move forward with submitting v9, once I rebase with the required changes for Eric's latest merge. Jason
On Tue, Nov 20, 2018 at 3:19 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > Yes. In fact it's used for FIPS certification testing. > Sure, nobody sane should be doing it. But when it comes to > government certification... :) The kernel does not aim toward any FIPS certification, and we're not going to start bloating our designs to fulfill this. It's never been a goal. Maybe ask Ted to add a FIPS mode to random.c and see what happens... When you start arguing "because FIPS!" as your justification, you really hit a head scratcher. > They've already paid for the indirect > function call so why make them go through yet another run-time > branch? The indirect function call in the crypto API is the performance hit. The branch in Zinc is not, as the predictor does the correct thing every single time. I'm not able to distinguish between the two looking at the performance measurements between it being there and the branch being commented out. Give me a few more days to finish v9's latest required changes for chacha12, and then I'll submit a revision that I think should address the remaining technical objections raised over the last several months we've been discussing this. Regards, Jason
On Tue, Nov 20, 2018 at 05:24:41PM +0100, Jason A. Donenfeld wrote: > On Tue, Nov 20, 2018 at 3:19 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Yes. In fact it's used for FIPS certification testing. > > Sure, nobody sane should be doing it. But when it comes to > > government certification... :) > > The kernel does not aim toward any FIPS certification, and we're not > going to start bloating our designs to fulfill this. It's never been a > goal. Maybe ask Ted to add a FIPS mode to random.c and see what > happens... When you start arguing "because FIPS!" as your > justification, you really hit a head scratcher. There are crazy people who go for FIPS certification for the kernel. That's why crypto/drbg.c exists. There is a crazy fips mode in drivers/char/random.c which causes the kernel to panic with a 1 in 2**80 probability each time _extract_entropy is called. It's not the default, and I have no idea if any one uses it, or if it's like the NIST OSI mandate, which forced everyone to buy an OSI networking stack --- and then put it on the shelf and use TCP/IP instead. Press release from March 2018: https://www.redhat.com/en/about/press-releases/red-hat-completes-fips-140-2-re-certification-red-hat-enterprise-linux-7 - Ted
On Tue, Nov 20, 2018 at 05:18:24PM +0100, Jason A. Donenfeld wrote: > > N'ack. As I mentioned in the last email, this really isn't okay, and > mostly defeats the purpose of Zinc in the first place, adding > complexity in a discombobulated half-baked way. Your proposal seems to > be the worst of all worlds. Having talked to lots of people about the > design of Zinc at this point to very positive reception, I'm going to > move forward with submitting v9, once I rebase with the required > changes for Eric's latest merge. Do you have any actual technical reasons for rejecting this apart from your apparent hatred of the existing crypto API? Cheers,
On Tue, Nov 20, 2018 at 05:24:41PM +0100, Jason A. Donenfeld wrote: > On Tue, Nov 20, 2018 at 3:19 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Yes. In fact it's used for FIPS certification testing. > > Sure, nobody sane should be doing it. But when it comes to > > government certification... :) > > The kernel does not aim toward any FIPS certification, and we're not > going to start bloating our designs to fulfill this. It's never been a > goal. Maybe ask Ted to add a FIPS mode to random.c and see what > happens... When you start arguing "because FIPS!" as your > justification, you really hit a head scratcher. FIPS is not the issue. The issue is that we need to keep the existing user-space ABI. We can't break that. Cheers,