| Message ID | 20180925145622.29959-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | WireGuard: Secure Network Tunnel | expand |
On Tue, Sep 25, 2018 at 04:55:59PM +0200, Jason A. Donenfeld wrote: > > It is intended that this entire patch series enter the kernel through > DaveM's net-next tree. Subsequently, WireGuard patches will go through > DaveM's net-next tree, while Zinc patches will go through Greg KH's tree. > Why is Herbert Xu's existing crypto tree being circumvented, especially for future patches (the initial merge isn't quite as important as that's a one-time event)? I like being able to check out cryptodev to test upcoming crypto patches. And currently, changes to APIs, algorithms, tests, and implementations all go through cryptodev, which is convenient for crypto developers. Apparently, you're proposing that someone adding a new algorithm will now have to submit the API portion to one maintainer (Herbert Xu) and the implementation portion to another maintainer (you), and they'll go through separate git trees. That's inconvenient for developers, and it seems that in practice you and Herbert will be stepping on each other's toes a lot. Can you please reach some kind of sane agreement with Herbert so that the development process isn't fractured into two? Perhaps you could review patches, but Herbert could still apply them? I'm also wondering about the criteria for making additions and changes to "Zinc". You mentioned before that one of the "advantages" of Zinc is that it doesn't include "cipher modes from 90s cryptographers" -- what does that mean exactly? You've also indicated before that you don't want people modifying the Poly1305 implementations as they are too error-prone. Yet apparently it's fine to change them yourself, e.g. when you added the part that converts the accumulator from base 26 to base 32. I worry there may be double standards here, and useful contributions could be blocked or discouraged in the future. Can you please elaborate on your criteria for contributions to Zinc? Also, will you allow algorithms that aren't up to modern security standards but are needed for compatibility reasons, e.g. MD5, SHA-1, and DES? There are existing standards, APIs, and data formats that use these "legacy" algorithms; so implementations of them are often still needed, whether we like it or not. And does it matter who designed the algorithms, e.g. do algorithms from Daniel Bernstein get effectively a free pass, while algorithms from certain countries, governments, or organizations are not allowed? E.g. wireless driver developers may need the SM4 block cipher (which is now supported by the crypto API) as it's specified in a Chinese wireless standard. Will you allow SM4 in Zinc? Or will people have to submit some algorithms to Herbert and some to you due to disagreements about what algorithms should be included? - Eric
Hi Eric, On Thu, Sep 27, 2018 at 8:29 PM Eric Biggers <ebiggers@kernel.org> wrote: > Why is Herbert Xu's existing crypto tree being circumvented, especially for > future patches (the initial merge isn't quite as important as that's a one-time > event)? I like being able to check out cryptodev to test upcoming crypto > patches. And currently, changes to APIs, algorithms, tests, and implementations > all go through cryptodev, which is convenient for crypto developers. > > Apparently, you're proposing that someone adding a new algorithm will now have > to submit the API portion to one maintainer (Herbert Xu) and the implementation > portion to another maintainer (you), and they'll go through separate git trees. > That's inconvenient for developers, and it seems that in practice you and > Herbert will be stepping on each other's toes a lot. > > Can you please reach some kind of sane agreement with Herbert so that the > development process isn't fractured into two? Perhaps you could review patches, > but Herbert could still apply them? I think you're overthinking it a bit. Zinc will have a few software implementations of primitives that are useful in cases where it's nice to call the primitive directly. Think: various usages of sha2, siphash, the wireguard suite (what this patchset includes), other things in lib/, etc. In so much as this winds up duplicating things within the crypto API, I'll work with Herbert to build one on top of the other -- as I've done in the two commits in this series. But beyond that, think of the two initiatives as orthogonal. I'm working on curating a few primitives that are maximally useful throughout the kernel for various uses, and doing so in a way that I think brings about a certain quality. Meanwhile the crypto API is amassing a huge collection of primitives for some things, and that will continue to exist, and Herbert will continue to maintain that. I expect for the crossover to be fairly isolated and manageable, without too much foreseeable tree- conflicts and such. Therefore, Samuel Neves and I plan to maintain the codebase we've spent quite some time writing, and maintain our own tree for it, which we'll be submitting through Greg. In other words, this is not a matter of "circumvention" or "stepping on toes", but rather separate efforts. I'm quite certain to the extent they overlap we'll be able to work out fairly easily. Either way, I'll take your suggestion and reach out to Herbert, since at least a discussion between the two of us sounds like it could be productive. > I'm also wondering about the criteria for making additions and changes to > "Zinc". You mentioned before that one of the "advantages" of Zinc is that it > doesn't include "cipher modes from 90s cryptographers" -- what does that mean > exactly? You've also indicated before that you don't want people modifying the > Poly1305 implementations as they are too error-prone. Useful contributions > could be blocked or discouraged in the future. Can you please elaborate on > your criteria for contributions to Zinc? > > Also, will you allow algorithms that aren't up to modern security standards but > are needed for compatibility reasons, e.g. MD5, SHA-1, and DES? There are > existing standards, APIs, and data formats that use these "legacy" algorithms; > so implementations of them are often still needed, whether we like it or not. > > And does it matter who designed the algorithms, e.g. do algorithms from Daniel > Bernstein get effectively a free pass, while algorithms from certain countries, > governments, or organizations are not allowed? E.g. wireless driver developers > may need the SM4 block cipher (which is now supported by the crypto API) as it's > specified in a Chinese wireless standard. Will you allow SM4 in Zinc? Or will > people have to submit some algorithms to Herbert and some to you due to > disagreements about what algorithms should be included? Similarly here, I think you're over-politicizing everything. Stable address generation for IPv6 uses SHA1 -- see net/ipv6/addrconf.c:3203 -- do you think that this should use, say, the SM3 chinese hash function instead? No, of course not, for a variety of interesting reasons. Rather, it should use some simple hash function that's fast in software that we have available in Zinc. On the other hand, it seems like parts of the kernel that have pretty high- levels of cipher agility -- such as dmcrypt, ipsec, wifi apparently, and so on -- will continue to use dynamic-dispatch system like the crypto API, since that's what it was made to do and is effective at doing. And so, your example of SM4 seems to fit perfectly into what the crypto API is well-suited for, and it would fit naturally in there. In other words, the "political criteria" for what we add to lib/zinc/ will mostly be the same as for the rest of lib/: are there things using it that benefit from it being there in a direct and obvious way, and does the implementation meet certain quality standards. > to change them yourself, e.g. when you added the part that converts the > accumulator from base 26 to base 32. I worry there may be double standards > here We do actually appreciate your concern here. However, there's a lot more that went into that short patch than meets the eye: - It matches exactly what Andy Polyakov's code is doing for the exact same reason, so this isn't something that's actually "new". (There are paths inside his implementation that branch from the vector code to the scalar code.) - It has been discussed at length with Andy, including what kinds of proofs we'll need if we want to chop it down further (to remove that final reduction), and why we both don't want to do that yet, and so we go with the full carrying for the avoidance of risk. - We've proved its correctness with Z3, actually using an even looser constraint on digit size than what Andy mentioned to us, thus resulting in a stronger proof result. So we're certain this isn't rubbish. - There's been some considerable computing power sunk into fuzzing it. There's no doubt about it, we've done our due-diligence here. This is in fact the kind of efforts we require of submissions. You could fault us for not detailing this in "the commit message" -- except as this is still a patch series, we're putting improvements into the 00/XX change log, instead of adding fixes and additions on top of the series. Of course in the ordinary course of kernel development, this would exist instead as a standalone commit. If you have a better idea of how this kind of thing can be communicated, and where precisely, in the pre-merge process, I'd be interested in hearing suggestions. Thanks, Jason
On Thu, Sep 27, 2018 at 11:35:39PM +0200, Jason A. Donenfeld wrote: > Hi Eric, > > On Thu, Sep 27, 2018 at 8:29 PM Eric Biggers <ebiggers@kernel.org> wrote: > > Why is Herbert Xu's existing crypto tree being circumvented, especially for > > future patches (the initial merge isn't quite as important as that's a one-time > > event)? I like being able to check out cryptodev to test upcoming crypto > > patches. And currently, changes to APIs, algorithms, tests, and implementations > > all go through cryptodev, which is convenient for crypto developers. > > > > Apparently, you're proposing that someone adding a new algorithm will now have > > to submit the API portion to one maintainer (Herbert Xu) and the implementation > > portion to another maintainer (you), and they'll go through separate git trees. > > That's inconvenient for developers, and it seems that in practice you and > > Herbert will be stepping on each other's toes a lot. > > > > Can you please reach some kind of sane agreement with Herbert so that the > > development process isn't fractured into two? Perhaps you could review patches, > > but Herbert could still apply them? > > I think you're overthinking it a bit. Zinc will have a few software > implementations of primitives that are useful in cases where it's nice to call > the primitive directly. Think: various usages of sha2, siphash, the wireguard > suite (what this patchset includes), other things in lib/, etc. In so much as > this winds up duplicating things within the crypto API, I'll work with Herbert > to build one on top of the other -- as I've done in the two commits in this > series. But beyond that, think of the two initiatives as orthogonal. I'm > working on curating a few primitives that are maximally useful throughout > the kernel for various uses, and doing so in a way that I think brings > about a certain quality. Meanwhile the crypto API is amassing a huge > collection of primitives for some things, and that will continue to exist, > and Herbert will continue to maintain that. I expect for the crossover > to be fairly isolated and manageable, without too much foreseeable tree- > conflicts and such. Therefore, Samuel Neves and I plan to maintain the > codebase we've spent quite some time writing, and maintain our own tree for > it, which we'll be submitting through Greg. In other words, this is not > a matter of "circumvention" or "stepping on toes", but rather separate > efforts. I'm quite certain to the extent they overlap we'll be able to work > out fairly easily. > > Either way, I'll take your suggestion and reach out to Herbert, since at > least a discussion between the two of us sounds like it could be productive. So, Zinc will simultaneously replace the current crypto implementations, *and* be "orthogonal" and "separate" from all the crypto code currently maintained by Herbert? You can't have your cake and eat it too... I'm still concerned you're splitting the community in two. It will be unclear where new algorithms and implementations should go. Some people will choose Herbert and the current crypto API and conventions, and some people will choose you and Zinc... I still don't see clear guidelines for what will go where. And yes, you and Herbert will step on each others' toes and duplicate stuff, as the efforts are *not* separate, as you've even argued yourself. Please reach out to Herbert to find a sane solution, ideally one that involves having a single git tree for crypto development and allows people to continue crypto development without choosing "sides". > > > I'm also wondering about the criteria for making additions and changes to > > "Zinc". You mentioned before that one of the "advantages" of Zinc is that it > > doesn't include "cipher modes from 90s cryptographers" -- what does that mean > > exactly? You've also indicated before that you don't want people modifying the > > Poly1305 implementations as they are too error-prone. Useful contributions > > could be blocked or discouraged in the future. Can you please elaborate on > > your criteria for contributions to Zinc? > > > > Also, will you allow algorithms that aren't up to modern security standards but > > are needed for compatibility reasons, e.g. MD5, SHA-1, and DES? There are > > existing standards, APIs, and data formats that use these "legacy" algorithms; > > so implementations of them are often still needed, whether we like it or not. > > > > And does it matter who designed the algorithms, e.g. do algorithms from Daniel > > Bernstein get effectively a free pass, while algorithms from certain countries, > > governments, or organizations are not allowed? E.g. wireless driver developers > > may need the SM4 block cipher (which is now supported by the crypto API) as it's > > specified in a Chinese wireless standard. Will you allow SM4 in Zinc? Or will > > people have to submit some algorithms to Herbert and some to you due to > > disagreements about what algorithms should be included? > > Similarly here, I think you're over-politicizing everything. Stable address > generation for IPv6 uses SHA1 -- see net/ipv6/addrconf.c:3203 -- do you think > that this should use, say, the SM3 chinese hash function instead? No, of > course not, for a variety of interesting reasons. Rather, it should use some > simple hash function that's fast in software that we have available in Zinc. > On the other hand, it seems like parts of the kernel that have pretty high- > levels of cipher agility -- such as dmcrypt, ipsec, wifi apparently, and > so on -- will continue to use dynamic-dispatch system like the crypto API, > since that's what it was made to do and is effective at doing. And so, your > example of SM4 seems to fit perfectly into what the crypto API is well-suited > for, and it would fit naturally in there. > > In other words, the "political criteria" for what we add to lib/zinc/ will > mostly be the same as for the rest of lib/: are there things using it that > benefit from it being there in a direct and obvious way, and does the > implementation meet certain quality standards. > So, crypto implementations and algorithms will go to different maintainers, source locations, and git trees based purely on whether the current users need "cipher agility"? Note that usage can change over time; a user that requires a single cipher could later need multiple, and vice versa. What if the portion of a wireless driver that needs SM4 doesn't need any other cipher in the same place, so static dispatch would suffice? Would SM4 be allowed in Zinc then? > > to change them yourself, e.g. when you added the part that converts the > > accumulator from base 26 to base 32. I worry there may be double standards > > here > > We do actually appreciate your concern here. However, there's a lot more that > went into that short patch than meets the eye: > > - It matches exactly what Andy Polyakov's code is doing for the exact > same reason, so this isn't something that's actually "new". (There > are paths inside his implementation that branch from the vector code > to the scalar code.) Matches Andy's code, where? The reason you had to add the radix conversion is because his code does *not* handle it... > - It has been discussed at length with Andy, including what kinds of > proofs we'll need if we want to chop it down further (to remove that > final reduction), and why we both don't want to do that yet, and so > we go with the full carrying for the avoidance of risk. Sorry, other people don't know about your private discussions. For the rest of us, why not add a comment to the code explaining what's going on? > - We've proved its correctness with Z3, actually using an even looser > constraint on digit size than what Andy mentioned to us, thus resulting > in a stronger proof result. So we're certain this isn't rubbish. AFAICS actually it *is* rubbish, because your C code stores the accumulator as 64-bit integers whereas the asm code (at least, the 32-bit version) reads it as 32-bit integers. That won't work correctly on big endian ARM. > - There's been some considerable computing power sunk into fuzzing it. > > There's no doubt about it, we've done our due-diligence here. Apparently not, given that it's broken on big endian ARM. > fact the kind of efforts we require of submissions. You could fault us for > not detailing this in "the commit message" -- except as this is still a > patch series, we're putting improvements into the 00/XX change log, instead > of adding fixes and additions on top of the series. The details of the correctness proofs and fuzzing you claim to have done aren't explained, even in the cover letter; so for now we just have to trust you on that point. Of course, having bugs in code which you insist was proven correct + fuzzed doesn't exactly inspire trust. I understand that your standards are still as high or even higher than Herbert's, which is good; crypto code should be held to high standards! But based on the evidence, I do worry there's a double standard going on where you get away with things yourself which you won't allow from others in Zinc. It's just not honest, and it will make people not want to contribute to Zinc. Maintainers are supposed to be unbiased and hold all contributions to the same standard. We need "Zinc" to be Linux's crypto library, not "Jason's crypto library". - Eric
Hi Eric, On Thu, Sep 27, 2018 at 06:17:27PM -0700, Eric Biggers wrote: > So, Zinc will simultaneously replace the current crypto implementations, *and* > be "orthogonal" and "separate" from all the crypto code currently maintained by > Herbert? You can't have your cake and eat it too... The plan is for it to replace many uses of the crypto API where it makes sense, but not replace uses where it doesn't make sense. Perhaps in the long run, over time, its usage will grow to cover those cases too, or, perhaps instead, Zinc will form a simple basis of software crypto algorithms in whatever future API designs crop up. In other words, like most changes in kernel development, things happen gradually, starting with a few good cases, and gradually growing as the need and design arise. > I'm still concerned you're splitting the community in two. It will be unclear > where new algorithms and implementations should go. Some people will choose > Herbert and the current crypto API and conventions, and some people will choose > you and Zinc... I still don't see clear guidelines for what will go where. I can try to work out some explicit guidelines and write these up for Documentation/, if that'd make a difference for you. I don't think this is a matter of "community splitting". On the contrary, I think Zinc will bring communities together, inviting the larger cryptography community to take an interest in improving the state of crypto in Linux. Either way, the litmus test for where code should go remains pretty similar to how it's been working so far. Are you tempted to stick it in lib/ because that fits your programming paradigm and because you think it's generally useful? If so, submit it to lib/zinc/. Conversely, is it only something used in the large array of options provided by dmcrypt, ipsec, afalg, etc? Submit it to the crypto API. If you think this criteria is lacking, I'm amenable to adjusting that and changing it, especially as situations and designs change and morph over time. But that seems like a fairly decent starting point. > Please reach out to Herbert to find a sane solution > crypto development without choosing "sides". Please, don't politicize this. This has nothing to do with "sides". This has to do with which paradigm makes sense for implementing a particular algorithm. And everything that goes in Zinc gets to be used seamlessly by the crypto API anyway, through use of the trivial stub glue code, like what I've shown in the two commits in this series. Again, if it's something that will work well as a direct function call, then it seems like Zinc makes sense as a home for it. With that said, I've reached out to Herbert, and we'll of course discuss and reach a good conclusion together. > Note that usage can change over time; a user that requires a > single cipher could later need multiple, and vice versa. I think this depends on the design of the driver and the style it's implemented in. For example, I could imagine something like this: encrypt_stuff_with_morus(obj, key); evolving over time to: if (obj->type == MORUS_TYPE) encrypt_stuff_with_morus(obj, key); else if (obj->type == AEGIS_TYPE) encrypt_stuff_with_aegis(obj, key); On the other hand, if the developer has good reason to use the crypto API's dynamic dispatch and async API and so forth, then perhaps it just changes from: static const char *cipher_name = "morus"; to static const char *cipher_name_type_1 = "morus"; static const char *cipher_name_type_2 = "aegis"; I can imagine both programming styles and evolutions being desirable for different reasons. > > - It matches exactly what Andy Polyakov's code is doing for the exact > > same reason, so this isn't something that's actually "new". (There > > are paths inside his implementation that branch from the vector code > > to the scalar code.) > > Matches Andy's code, where? The reason you had to add the radix conversion is > because his code does *not* handle it... For example, check out the avx blocks function. The radix conversion happens in a few different places throughout. The reason we need it separately here is because, unlike userspace, it's possible the kernel code will transition from 2^26 back to 2^64 as a result of the FPU context changing. As well, AndyP seems to like the idea of including this logic in the assembly instead of in C, if I understood our discussions correctly, so there's a decent chance this will migrate out of the glue code and into the assembly properly, which is probably a better place for it. > > - It has been discussed at length with Andy, including what kinds of > > proofs we'll need if we want to chop it down further (to remove that > > final reduction), and why we both don't want to do that yet, and so > > we go with the full carrying for the avoidance of risk. > > Sorry, other people don't know about your private discussions. For the rest of > us, why not add a comment to the code explaining what's going on? That's a good idea. I can include some discussion about this as well in the commit message that introduces the glue code, too, I guess? I've been hesitant to fill these commit messages up even more, given there are already so many walls of text and whatnot, but if you think that'd be useful, I'll do that for v7, and also add comments. > > - We've proved its correctness with Z3, actually using an even looser > > constraint on digit size than what Andy mentioned to us, thus resulting > > in a stronger proof result. So we're certain this isn't rubbish. > AFAICS actually it *is* rubbish, because your C code stores the accumulator as > 64-bit integers whereas the asm code (at least, the 32-bit version) reads it as > 32-bit integers. That won't work correctly on big endian ARM. > > There's no doubt about it, we've done our due-diligence here. > Apparently not, given that it's broken on big endian ARM. > Of course, having bugs in code which you insist was proven correct > + fuzzed doesn't exactly inspire trust. What's with the snark? It's not rubbish. I'm not sure if you noticed it in the development trees (both the WireGuard module tree and my kernel.org integration tree for this patch), but the big endian ARM support was fixed pretty shortly after I jumped the gun posting v6. Like, super soon after. That, and other big endian fixes (on aarch64 as well) are already queued up for v7. And now build.wireguard.com has more big endian running in CI. > The details of the correctness proofs and fuzzing you claim to have done aren't > explained, even in the cover letter; so for now we just have to trust you on > that point. "Claim to have done", "trust you on that point" -- I think there's no reason to doubt the integrity of my "claims", and I don't appreciate the phrasing that appears to call that into question. Regardless, sure, we can expand the "wall-of-text" commit messages even further, if you want, and include the verbatim Z3 scripts for reproduction. > I understand that your standards are still as high or even higher than > Herbert's, which is good; crypto code should be held to high standards! But > based on the evidence, I do worry there's a double standard going on where you > get away with things yourself which you won't allow from others in Zinc. It's > just not honest, and it will make people not want to contribute to Zinc. > Maintainers are supposed to be unbiased and hold all contributions to the same > standard. This is complete and utter garbage, and I find its insinuations insulting and ridiculous. There is absolutely no lack of honesty and no double standard being applied whatsoever. Your attempt to cast doubt about the quality of standards applied and the integrity of the process is wholly inappropriate. When I tell you that high standards were applied and that due-diligence was done in developing a particular patch, I mean what I say. > We need "Zinc" to be Linux's crypto library, not "Jason's crypto library". This very much is a project directed toward the benefit of the kernel in a general sense. It's been this way from the start, and there's nothing in its goals or plans to the contrary of that. Please leave this vague and unproductive rhetoric aside. Jason
Hi Jason, On Fri, Sep 28, 2018 at 04:35:48AM +0200, Jason A. Donenfeld wrote: > Hi Eric, > > On Thu, Sep 27, 2018 at 06:17:27PM -0700, Eric Biggers wrote: > > So, Zinc will simultaneously replace the current crypto implementations, *and* > > be "orthogonal" and "separate" from all the crypto code currently maintained by > > Herbert? You can't have your cake and eat it too... > > The plan is for it to replace many uses of the crypto API where it makes > sense, but not replace uses where it doesn't make sense. Perhaps in the > long run, over time, its usage will grow to cover those cases too, or, > perhaps instead, Zinc will form a simple basis of software crypto > algorithms in whatever future API designs crop up. In other words, like > most changes in kernel development, things happen gradually, starting > with a few good cases, and gradually growing as the need and design > arise. Please re-read what I wrote. Here I'm talking about the crypto code itself, not its users. And you still haven't answered my question about adding a new algorithm that is useful to have in both APIs. You're proposing that in most cases the crypto API part will need to go through Herbert while the implementation will need to go through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take both? > > > I'm still concerned you're splitting the community in two. It will be unclear > > where new algorithms and implementations should go. Some people will choose > > Herbert and the current crypto API and conventions, and some people will choose > > you and Zinc... I still don't see clear guidelines for what will go where. > > I can try to work out some explicit guidelines and write these up for > Documentation/, if that'd make a difference for you. I don't think this > is a matter of "community splitting". On the contrary, I think Zinc will > bring communities together, inviting the larger cryptography community > to take an interest in improving the state of crypto in Linux. Either > way, the litmus test for where code should go remains pretty similar to > how it's been working so far. Are you tempted to stick it in lib/ > because that fits your programming paradigm and because you think it's > generally useful? If so, submit it to lib/zinc/. Conversely, is it only > something used in the large array of options provided by dmcrypt, ipsec, > afalg, etc? Submit it to the crypto API. > > If you think this criteria is lacking, I'm amenable to adjusting that > and changing it, especially as situations and designs change and morph > over time. But that seems like a fairly decent starting point. A documentation file for Zinc is a good idea. Some of the information in your commit messages should be moved there too. > > > Please reach out to Herbert to find a sane solution > > crypto development without choosing "sides". > > Please, don't politicize this. This has nothing to do with "sides". This > has to do with which paradigm makes sense for implementing a particular > algorithm. I'm not trying to "politicize" this, but rather determine your criteria for including algorithms in Zinc, which you haven't made clear. Since for the second time you've avoided answering my question about whether you'd allow the SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically opinionated", and you were one of the loudest voices calling for the Speck cipher to be removed, and your justification for Zinc complains about cipher modes from "90s cryptographers", I think it's reasonable for people to wonder whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the inclusion of crypto algorithms to the ones you prefer, rather than the ones that users need. Note that the kernel is used by people all over the world and needs to support various standards, protocols, and APIs that use different crypto algorithms, not always the ones we'd like; and different users have different preferences. People need to know whether the Linux kernel's crypto library will meet (or be allowed to meet) their crypto needs. > And everything that goes in Zinc gets to be used seamlessly > by the crypto API anyway, through use of the trivial stub glue code, > like what I've shown in the two commits in this series. Again, if it's > something that will work well as a direct function call, then it seems > like Zinc makes sense as a home for it. > > With that said, I've reached out to Herbert, and we'll of course discuss > and reach a good conclusion together. > > > Note that usage can change over time; a user that requires a > > single cipher could later need multiple, and vice versa. > > I think this depends on the design of the driver and the style it's > implemented in. For example, I could imagine something like this: > > encrypt_stuff_with_morus(obj, key); > > evolving over time to: > > if (obj->type == MORUS_TYPE) > encrypt_stuff_with_morus(obj, key); > else if (obj->type == AEGIS_TYPE) > encrypt_stuff_with_aegis(obj, key); > > On the other hand, if the developer has good reason to use the crypto > API's dynamic dispatch and async API and so forth, then perhaps it just > changes from: > > static const char *cipher_name = "morus"; > > to > > static const char *cipher_name_type_1 = "morus"; > static const char *cipher_name_type_2 = "aegis"; > > I can imagine both programming styles and evolutions being desirable for > different reasons. > > > > - It matches exactly what Andy Polyakov's code is doing for the exact > > > same reason, so this isn't something that's actually "new". (There > > > are paths inside his implementation that branch from the vector code > > > to the scalar code.) > > > > Matches Andy's code, where? The reason you had to add the radix conversion is > > because his code does *not* handle it... > > For example, check out the avx blocks function. The radix conversion > happens in a few different places throughout. The reason we need it > separately here is because, unlike userspace, it's possible the kernel > code will transition from 2^26 back to 2^64 as a result of the FPU > context changing. IOW, you had to rewrite the x86 assembly algorithm in C and make it use a different Poly1305 context format. That's a major change, where bugs can be introduced -- and at least one was introduced, in fact. I'd appreciate it if you were more accurate in describing your modifications and the potential risks involved. And yes I know why converting radixes is needed, as I had pointed it out... > > As well, AndyP seems to like the idea of including this logic in the > assembly instead of in C, if I understood our discussions correctly, so > there's a decent chance this will migrate out of the glue code and into > the assembly properly, which is probably a better place for it. > > > > - It has been discussed at length with Andy, including what kinds of > > > proofs we'll need if we want to chop it down further (to remove that > > > final reduction), and why we both don't want to do that yet, and so > > > we go with the full carrying for the avoidance of risk. > > > > Sorry, other people don't know about your private discussions. For the rest of > > us, why not add a comment to the code explaining what's going on? > > That's a good idea. I can include some discussion about this as well in > the commit message that introduces the glue code, too, I guess? I've > been hesitant to fill these commit messages up even more, given there > are already so many walls of text and whatnot, but if you think that'd > be useful, I'll do that for v7, and also add comments. Please prefer to put information in the code or documentation rather than in commit messages, when appropriate. > > > > - We've proved its correctness with Z3, actually using an even looser > > > constraint on digit size than what Andy mentioned to us, thus resulting > > > in a stronger proof result. So we're certain this isn't rubbish. > > AFAICS actually it *is* rubbish, because your C code stores the accumulator as > > 64-bit integers whereas the asm code (at least, the 32-bit version) reads it as > > 32-bit integers. That won't work correctly on big endian ARM. > > > There's no doubt about it, we've done our due-diligence here. > > Apparently not, given that it's broken on big endian ARM. > > Of course, having bugs in code which you insist was proven correct > > + fuzzed doesn't exactly inspire trust. > > What's with the snark? It's not rubbish. I'm not sure if you noticed it in > the development trees (both the WireGuard module tree and my kernel.org > integration tree for this patch), but the big endian ARM support was fixed > pretty shortly after I jumped the gun posting v6. Like, super soon after. > That, and other big endian fixes (on aarch64 as well) are already queued up > for v7. And now build.wireguard.com has more big endian running in CI. > > > The details of the correctness proofs and fuzzing you claim to have done aren't > > explained, even in the cover letter; so for now we just have to trust you on > > that point. > > "Claim to have done", "trust you on that point" -- I think there's no > reason to doubt the integrity of my "claims", and I don't appreciate the > phrasing that appears to call that into question. > > Regardless, sure, we can expand the "wall-of-text" commit messages even > further, if you want, and include the verbatim Z3 scripts for reproduction. > > > I understand that your standards are still as high or even higher than > > Herbert's, which is good; crypto code should be held to high standards! But > > based on the evidence, I do worry there's a double standard going on where you > > get away with things yourself which you won't allow from others in Zinc. It's > > just not honest, and it will make people not want to contribute to Zinc. > > Maintainers are supposed to be unbiased and hold all contributions to the same > > standard. > > This is complete and utter garbage, and I find its insinuations insulting > and ridiculous. There is absolutely no lack of honesty and no double > standard being applied whatsoever. Your attempt to cast doubt about the > quality of standards applied and the integrity of the process is wholly > inappropriate. When I tell you that high standards were applied and that > due-diligence was done in developing a particular patch, I mean what I > say. No, I was not aware of the fixed version. I found the bug myself reading the latest patchset you've mailed out, v6. It's great that you found and fixed this bug on your own, and of course that raises my level of confidence in your work. Still, the v6 patchset still includes your claim that "All implementations have been extensively tested and fuzzed"; so that claim was objectively wrong. So I disagree that my concerns are "complete and utter garbage". And I think that the fact that you prefer to respond by attacking me, rather than committing to be more accurate in your claims and to treat all contributions fairly, is problematic. Also, if you have extra tests that you're running, it would be great if you could include them as part of the submission so that others can run them too. > > > We need "Zinc" to be Linux's crypto library, not "Jason's crypto library". > > This very much is a project directed toward the benefit of the kernel in > a general sense. It's been this way from the start, and there's nothing > in its goals or plans to the contrary of that. Please leave this vague > and unproductive rhetoric aside. > Well, it doesn't help that you yourself claim that Zinc stands for "Zx2c4's INsane Cryptolib"... - Eric
Hi Eric, On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@kernel.org> wrote: > And you still haven't answered my question about adding a new algorithm that is > useful to have in both APIs. You're proposing that in most cases the crypto API > part will need to go through Herbert while the implementation will need to go > through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take > both? If an implementation enters Zinc, it will go through my tree. If it enters the crypto API, it will go through Herbert's tree. If there wind up being messy tree dependency and merge timing issues, I can work this out in the usual way with Herbert. I'll be sure to discuss these edge cases with him when we discuss. I think there's a way to handle that with minimum friction. > A documentation file for Zinc is a good idea. Some of the information in your > commit messages should be moved there too. Will do. > I'm not trying to "politicize" this, but rather determine your criteria for > including algorithms in Zinc, which you haven't made clear. Since for the > second time you've avoided answering my question about whether you'd allow the > SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically > opinionated", and you were one of the loudest voices calling for the Speck > cipher to be removed, and your justification for Zinc complains about cipher > modes from "90s cryptographers", I think it's reasonable for people to wonder > whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the > inclusion of crypto algorithms to the ones you prefer, rather than the ones that > users need. Note that the kernel is used by people all over the world and needs > to support various standards, protocols, and APIs that use different crypto > algorithms, not always the ones we'd like; and different users have different > preferences. People need to know whether the Linux kernel's crypto library will > meet (or be allowed to meet) their crypto needs. WireGuard is indeed quite opinionated in its primitive choices, but I don't think it'd be wise to apply the same design to Zinc. There are APIs where the goal is to have a limited set of high-level functions, and have those implemented in only a preferred set of primitives. NaCl is a good example of this -- functions like "crypto_secretbox" that are actually xsalsapoly under the hood. Zinc doesn't intend to become an API like those, but rather to provide the actual primitives for use cases where that specific primitive is used. Sometimes the kernel is in the business of being able to pick its own crypto -- random.c, tcp sequence numbers, big_key.c, etc -- but most of the time the kernel is in the business of implementing other people's crypto, for specific devices/protocols/diskformats. And for those use cases we need not some high-level API like NaCl, but rather direct access to the primitives that are required to implement those drivers. WireGuard is one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so on. We're in the business of writing drivers, after all. So, no, I don't think I'd knock down the addition of a primitive because of a simple preference for a different primitive, if it was clearly the case that the driver requiring it really benefited from having accessible via the plain Zinc function calls. Sorry if I hadn't made this clear earlier -- I thought Ard had asked more or less the same thing about DES and I answered accordingly, but maybe that wasn't made clear enough there. > > For example, check out the avx blocks function. The radix conversion > > happens in a few different places throughout. The reason we need it > > separately here is because, unlike userspace, it's possible the kernel > > code will transition from 2^26 back to 2^64 as a result of the FPU > > context changing. > > IOW, you had to rewrite the x86 assembly algorithm in C and make it use a > different Poly1305 context format. That's a major change, where bugs can be > introduced -- and at least one was introduced, in fact. I'd appreciate it if > you were more accurate in describing your modifications and the potential risks > involved. A different Poly1305 context format? Not at all - it's using the exact same context struct as the assembly. And it's making the same conversion that the assembly is. There's not something "different" happening; that's the whole point. Also, this is not some process of frightfully transcribing assembly to C and hoping it all works out. This is a careful process of reasoning about the limbs, proving that the carries are correct, and that the arithmetic done in C actually corresponds to what we want. And then finally we check that what we've implemented is indeed the same as what the assembly implemented. Finally, as I mentioned, hopefully Andy will be folding this back into his implementations sometime soon anyway. > > That's a good idea. I can include some discussion about this as well in > > the commit message that introduces the glue code, too, I guess? I've > > been hesitant to fill these commit messages up even more, given there > > are already so many walls of text and whatnot, but if you think that'd > > be useful, I'll do that for v7, and also add comments. > > Please prefer to put information in the code or documentation rather than in > commit messages, when appropriate. Okay, no problem. > > This is complete and utter garbage, and I find its insinuations insulting > > and ridiculous. There is absolutely no lack of honesty and no double > > standard being applied whatsoever. Your attempt to cast doubt about the > > quality of standards applied and the integrity of the process is wholly > > inappropriate. When I tell you that high standards were applied and that > > due-diligence was done in developing a particular patch, I mean what I > > say. > > I > disagree that my concerns are "complete and utter garbage". And I think that > the fact that you prefer to respond by attacking me, rather than committing to > be more accurate in your claims and to treat all contributions fairly, is > problematic. I believe you have the sequence of events wrong. I've stated and made that there isn't a double standard, that the standards intend to be evenly rigorous, and I solicited your feedback on how I could best communicate changes in pre-merged patch series; I did the things you've said one should do. But your rhetoric then moved to questioning my integrity, and I believe that was uncalled for, inappropriate, and not a useful contribution to this mostly productive discussion -- hence garbage. In other words, I provided an acceptable defense, not an attack. But can we move past all this schoolyard nonsense? Cut the rhetoric, please; it's really quite overwhelming. > It's great that you found and fixed this > bug on your own, and of course that raises my level of confidence in your work. Good. > Still, the v6 patchset still includes your claim that "All implementations have > been extensively tested and fuzzed"; so that claim was objectively wrong. I don't think that claim is wrong. The fuzzing and testing that's been done has been extensive, and the fuzzing and testing that continues to occur is extensive. As mentioned, the bug was fixed pretty much right after git-send-email. If you'd like I can try to space out the patch submissions by a little bit longer -- that would probably have various benefits -- but given that the netdev code is yet to receive a thorough review, I think we've got a bit of time before this is merged. The faster-paced patch cycles might inadvertently introduce things that get fixed immediately after sending then, unfortunately, but I don't think this will be the case with the final series that's merged. Though I'm incorporating tons and tons of feedback right now, I do look forward to the structure of the series settling down a little bit and the pace of suggestions decreasing, so that I can focus on auditing and verifying again. But given the dynamism and interesting insights of the reviews so far, I think the fast pace has actually been useful for elucidating the various expectations and suggestions of reviewers. It's most certainly improved this patchset in terrific ways. > Well, it doesn't help that you yourself claim that Zinc stands for > "Zx2c4's INsane Cryptolib"... This expansion of the acronym was intended as a totally ridiculous joke. I guess it wasn't received well. I'll remove it from the next series. Sorry. Jason
On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hi Eric, > > On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@kernel.org> wrote: >> And you still haven't answered my question about adding a new algorithm that is >> useful to have in both APIs. You're proposing that in most cases the crypto API >> part will need to go through Herbert while the implementation will need to go >> through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take >> both? > > If an implementation enters Zinc, it will go through my tree. If it > enters the crypto API, it will go through Herbert's tree. If there > wind up being messy tree dependency and merge timing issues, I can > work this out in the usual way with Herbert. I'll be sure to discuss > these edge cases with him when we discuss. I think there's a way to > handle that with minimum friction. > I would also strongly prefer that all crypto work is taken through Herbert's tree, so we have a coherent view of it before it goes upstream. >> A documentation file for Zinc is a good idea. Some of the information in your >> commit messages should be moved there too. > > Will do. > >> I'm not trying to "politicize" this, but rather determine your criteria for >> including algorithms in Zinc, which you haven't made clear. Since for the >> second time you've avoided answering my question about whether you'd allow the >> SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically >> opinionated", and you were one of the loudest voices calling for the Speck >> cipher to be removed, and your justification for Zinc complains about cipher >> modes from "90s cryptographers", I think it's reasonable for people to wonder >> whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the >> inclusion of crypto algorithms to the ones you prefer, rather than the ones that >> users need. Note that the kernel is used by people all over the world and needs >> to support various standards, protocols, and APIs that use different crypto >> algorithms, not always the ones we'd like; and different users have different >> preferences. People need to know whether the Linux kernel's crypto library will >> meet (or be allowed to meet) their crypto needs. > > WireGuard is indeed quite opinionated in its primitive choices, but I > don't think it'd be wise to apply the same design to Zinc. There are > APIs where the goal is to have a limited set of high-level functions, > and have those implemented in only a preferred set of primitives. NaCl > is a good example of this -- functions like "crypto_secretbox" that > are actually xsalsapoly under the hood. Zinc doesn't intend to become > an API like those, but rather to provide the actual primitives for use > cases where that specific primitive is used. Sometimes the kernel is > in the business of being able to pick its own crypto -- random.c, tcp > sequence numbers, big_key.c, etc -- but most of the time the kernel is > in the business of implementing other people's crypto, for specific > devices/protocols/diskformats. And for those use cases we need not > some high-level API like NaCl, but rather direct access to the > primitives that are required to implement those drivers. WireGuard is > one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so > on. We're in the business of writing drivers, after all. So, no, I > don't think I'd knock down the addition of a primitive because of a > simple preference for a different primitive, if it was clearly the > case that the driver requiring it really benefited from having > accessible via the plain Zinc function calls. Sorry if I hadn't made > this clear earlier -- I thought Ard had asked more or less the same > thing about DES and I answered accordingly, but maybe that wasn't made > clear enough there. > >> > For example, check out the avx blocks function. The radix conversion >> > happens in a few different places throughout. The reason we need it >> > separately here is because, unlike userspace, it's possible the kernel >> > code will transition from 2^26 back to 2^64 as a result of the FPU >> > context changing. >> >> IOW, you had to rewrite the x86 assembly algorithm in C and make it use a >> different Poly1305 context format. That's a major change, where bugs can be >> introduced -- and at least one was introduced, in fact. I'd appreciate it if >> you were more accurate in describing your modifications and the potential risks >> involved. > > A different Poly1305 context format? Not at all - it's using the exact > same context struct as the assembly. And it's making the same > conversion that the assembly is. There's not something "different" > happening; that's the whole point. > > Also, this is not some process of frightfully transcribing assembly to > C and hoping it all works out. This is a careful process of reasoning > about the limbs, proving that the carries are correct, and that the > arithmetic done in C actually corresponds to what we want. And then > finally we check that what we've implemented is indeed the same as > what the assembly implemented. Finally, as I mentioned, hopefully Andy > will be folding this back into his implementations sometime soon > anyway. > >> > That's a good idea. I can include some discussion about this as well in >> > the commit message that introduces the glue code, too, I guess? I've >> > been hesitant to fill these commit messages up even more, given there >> > are already so many walls of text and whatnot, but if you think that'd >> > be useful, I'll do that for v7, and also add comments. >> >> Please prefer to put information in the code or documentation rather than in >> commit messages, when appropriate. > > Okay, no problem. > >> > This is complete and utter garbage, and I find its insinuations insulting >> > and ridiculous. There is absolutely no lack of honesty and no double >> > standard being applied whatsoever. Your attempt to cast doubt about the >> > quality of standards applied and the integrity of the process is wholly >> > inappropriate. When I tell you that high standards were applied and that >> > due-diligence was done in developing a particular patch, I mean what I >> > say. >> >> I >> disagree that my concerns are "complete and utter garbage". And I think that >> the fact that you prefer to respond by attacking me, rather than committing to >> be more accurate in your claims and to treat all contributions fairly, is >> problematic. > > I believe you have the sequence of events wrong. I've stated and made > that there isn't a double standard, that the standards intend to be > evenly rigorous, and I solicited your feedback on how I could best > communicate changes in pre-merged patch series; I did the things > you've said one should do. But your rhetoric then moved to questioning > my integrity, and I believe that was uncalled for, inappropriate, and > not a useful contribution to this mostly productive discussion -- > hence garbage. In other words, I provided an acceptable defense, not > an attack. But can we move past all this schoolyard nonsense? Cut the > rhetoric, please; it's really quite overwhelming. > >> It's great that you found and fixed this >> bug on your own, and of course that raises my level of confidence in your work. > > Good. > >> Still, the v6 patchset still includes your claim that "All implementations have >> been extensively tested and fuzzed"; so that claim was objectively wrong. > > I don't think that claim is wrong. The fuzzing and testing that's been > done has been extensive, and the fuzzing and testing that continues to > occur is extensive. As mentioned, the bug was fixed pretty much right > after git-send-email. If you'd like I can try to space out the patch > submissions by a little bit longer -- that would probably have various > benefits -- but given that the netdev code is yet to receive a > thorough review, I think we've got a bit of time before this is > merged. The faster-paced patch cycles might inadvertently introduce > things that get fixed immediately after sending then, unfortunately, > but I don't think this will be the case with the final series that's > merged. Though I'm incorporating tons and tons of feedback right now, > I do look forward to the structure of the series settling down a > little bit and the pace of suggestions decreasing, so that I can focus > on auditing and verifying again. But given the dynamism and > interesting insights of the reviews so far, I think the fast pace has > actually been useful for elucidating the various expectations and > suggestions of reviewers. It's most certainly improved this patchset > in terrific ways. > >> Well, it doesn't help that you yourself claim that Zinc stands for >> "Zx2c4's INsane Cryptolib"... > > This expansion of the acronym was intended as a totally ridiculous > joke. I guess it wasn't received well. I'll remove it from the next > series. Sorry. > As I understood from someone who was at your Kernel Recipes talk, you mentioned that it actually stands for 'zinc is not crypto/' (note the slash) That is needlessly divisive and condescending, so if you want to move past the rhetoric and the schoolyard nonsense, perhaps you can find a better name for it? Or just put your stuff into crypto/base/, crypto/core/ or something like that.
On Fri, Sep 28, 2018 at 9:52 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > As I understood from someone who was at your Kernel Recipes talk, you > mentioned that it actually stands for 'zinc is not crypto/' (note the > slash) I mentioned this was in v1 but it wasn't taken as lightly as planned and was removed, so if it needs a recursive acronym it can be "zinc is nice crypto", "zinc is neat crypto", or as the commit message mentions, "zinc as in crypto." Alternatively, it can just not stand for anything at all. Zinc -> Zinc. That's what the thing is called.
On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hi Eric, > > On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@kernel.org> wrote: >> And you still haven't answered my question about adding a new algorithm that is >> useful to have in both APIs. You're proposing that in most cases the crypto API >> part will need to go through Herbert while the implementation will need to go >> through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take >> both? > > If an implementation enters Zinc, it will go through my tree. If it > enters the crypto API, it will go through Herbert's tree. If there > wind up being messy tree dependency and merge timing issues, I can > work this out in the usual way with Herbert. I'll be sure to discuss > these edge cases with him when we discuss. I think there's a way to > handle that with minimum friction. > >> A documentation file for Zinc is a good idea. Some of the information in your >> commit messages should be moved there too. > > Will do. > >> I'm not trying to "politicize" this, but rather determine your criteria for >> including algorithms in Zinc, which you haven't made clear. Since for the >> second time you've avoided answering my question about whether you'd allow the >> SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically >> opinionated", and you were one of the loudest voices calling for the Speck >> cipher to be removed, and your justification for Zinc complains about cipher >> modes from "90s cryptographers", I think it's reasonable for people to wonder >> whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the >> inclusion of crypto algorithms to the ones you prefer, rather than the ones that >> users need. Note that the kernel is used by people all over the world and needs >> to support various standards, protocols, and APIs that use different crypto >> algorithms, not always the ones we'd like; and different users have different >> preferences. People need to know whether the Linux kernel's crypto library will >> meet (or be allowed to meet) their crypto needs. > > WireGuard is indeed quite opinionated in its primitive choices, but I > don't think it'd be wise to apply the same design to Zinc. There are > APIs where the goal is to have a limited set of high-level functions, > and have those implemented in only a preferred set of primitives. NaCl > is a good example of this -- functions like "crypto_secretbox" that > are actually xsalsapoly under the hood. Zinc doesn't intend to become > an API like those, but rather to provide the actual primitives for use > cases where that specific primitive is used. Sometimes the kernel is > in the business of being able to pick its own crypto -- random.c, tcp > sequence numbers, big_key.c, etc -- but most of the time the kernel is > in the business of implementing other people's crypto, for specific > devices/protocols/diskformats. And for those use cases we need not > some high-level API like NaCl, but rather direct access to the > primitives that are required to implement those drivers. WireGuard is > one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so > on. We're in the business of writing drivers, after all. So, no, I > don't think I'd knock down the addition of a primitive because of a > simple preference for a different primitive, if it was clearly the > case that the driver requiring it really benefited from having > accessible via the plain Zinc function calls. Sorry if I hadn't made > this clear earlier -- I thought Ard had asked more or less the same > thing about DES and I answered accordingly, but maybe that wasn't made > clear enough there. > CRC32 is another case study that I would like to bring up: - the current status is a bit of a mess, where we treat crc32() and crc32c() differently - the latter is exposed by libcrc32.ko as a wrapper around the crypto API, and there is some networking code (SCTP iirc) that puts another kludge on top of that to ensure that the code is not used before the module is loaded - we have C versions, scalar hw instruction based versions and carryless multiplication based versions for various architectures - on ST platforms, we have a synchronous hw accelerator that is 10x faster than C code on the same platform. AFAICT none of its current users rely on the async interface or runtime dispatch of the 'hash'. CRC32/c is an area that I would *really* like to clean up (and am already doing for arm64) - just having a function call interface would be a huge improvement but it seems to me that the choice for monolithic modules per algo/architecture implies that we will have to leave ST behind in this case. On the one hand, disregarding this seems fair, at least for the moment. On the other hand, fixing this in the crypto API, e.g., by permitting direct function calls to synchronous hashes and without the need to allocate a transform/request etc, blurs the line even more where different pieces should live. Any thoughts?
[+Willy] Hi Ard, On Fri, Sep 28, 2018 at 7:47 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > WireGuard is indeed quite opinionated in its primitive choices, but I > > don't think it'd be wise to apply the same design to Zinc. There are > > APIs where the goal is to have a limited set of high-level functions, > > and have those implemented in only a preferred set of primitives. NaCl > > is a good example of this -- functions like "crypto_secretbox" that > > are actually xsalsapoly under the hood. Zinc doesn't intend to become > > an API like those, but rather to provide the actual primitives for use > > cases where that specific primitive is used. Sometimes the kernel is > > in the business of being able to pick its own crypto -- random.c, tcp > > sequence numbers, big_key.c, etc -- but most of the time the kernel is > > in the business of implementing other people's crypto, for specific > > devices/protocols/diskformats. And for those use cases we need not > > some high-level API like NaCl, but rather direct access to the > > primitives that are required to implement those drivers. WireGuard is > > one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so > > on. We're in the business of writing drivers, after all. So, no, I > > don't think I'd knock down the addition of a primitive because of a > > simple preference for a different primitive, if it was clearly the > > case that the driver requiring it really benefited from having > > accessible via the plain Zinc function calls. Sorry if I hadn't made > > this clear earlier -- I thought Ard had asked more or less the same > > thing about DES and I answered accordingly, but maybe that wasn't made > > clear enough there. > > > > CRC32 is another case study that I would like to bring up: > - the current status is a bit of a mess, where we treat crc32() and > crc32c() differently - the latter is exposed by libcrc32.ko as a > wrapper around the crypto API, and there is some networking code (SCTP > iirc) that puts another kludge on top of that to ensure that the code > is not used before the module is loaded > - we have C versions, scalar hw instruction based versions and > carryless multiplication based versions for various architectures > - on ST platforms, we have a synchronous hw accelerator that is 10x > faster than C code on the same platform. > > AFAICT none of its current users rely on the async interface or > runtime dispatch of the 'hash'. I've added Willy to the CC, as we were actually discussing the crc32 case together two days ago. If my understanding was correct, he seemed to think it'd be pretty useful too. It seems like unifying the crc32 implementations at some point would make sense, and since there's no usage of these as part of the crypto api, providing crc32 via a vanilla function call seems reasonable. It's not clear that on some strict formal level, crc32 belongs anywhere near Zinc, since it's not _exactly_ in the same category... But one could broaden the scope a bit and make the argument that this general idea of accepting some bytes, doing some "pure" arithmetic operations from a particular discipline on them in slightly different ways depending on the architecture, and then returning some other bytes, is what in essence is happening with these all of these function calls, no matter their security or intended use; so if crc32 would benefit from being implemented using the exact same design patterns, then it might as well be grouped in with the rest of Zinc. On the other hand, this might be a bit of a slippery slope ("are compression functions next?"), and a libcrc32.ko could just as well exist as a standalone thing. I can see it both ways and some other arguments too, but also this might form a good setting for some much needed cleanup of the crc32. So that's definitely something we can consider. Regarding the synchronous ST driver: I'll give it a thorough read through, but I do wonder off the bat if actually it'd be possible to incorporate that into the Zinc paradigm in a fairly non-intrusive way. I'll give that some thought and play around a bit with it. Jason
On Sat, Sep 29, 2018 at 04:40:03AM +0200, Jason A. Donenfeld wrote: > > CRC32 is another case study that I would like to bring up: > > - the current status is a bit of a mess, where we treat crc32() and > > crc32c() differently - the latter is exposed by libcrc32.ko as a > > wrapper around the crypto API, and there is some networking code (SCTP > > iirc) that puts another kludge on top of that to ensure that the code > > is not used before the module is loaded > > - we have C versions, scalar hw instruction based versions and > > carryless multiplication based versions for various architectures > > - on ST platforms, we have a synchronous hw accelerator that is 10x > > faster than C code on the same platform. > > > > AFAICT none of its current users rely on the async interface or > > runtime dispatch of the 'hash'. > > I've added Willy to the CC, as we were actually discussing the crc32 > case together two days ago. If my understanding was correct, he seemed > to think it'd be pretty useful too. Yep, I've had a hard time finding a suitable implementation when I wrote libslz (deflate+gzip compression). Some were totally inefficient in C, spending their time negating the value twice for each byte because they were using the pre-computed tables found in literature, some were using optimized SIMD and I didn't understand them at all. Finally I spent some time studying the principle and implemented mine. Later I discovered that ARMv8 chips have a crc32 instruction for this which is very fast. I tried it and it works much faster than my C implementation. For me it's a perfect example of something that could go into a lib like zinc. > It seems like unifying the crc32 implementations at some point would > make sense, and since there's no usage of these as part of the crypto > api, providing crc32 via a vanilla function call seems reasonable. > It's not clear that on some strict formal level, crc32 belongs > anywhere near Zinc, since it's not _exactly_ in the same category... Well, it's a hash. Not a cryptographically secure one (32 bits only) but definitely a hash usable (and used) for many cases. Those who need a hash could start with SHA2 and figure that they don't need something that secure since they only use 32 output bits and be fine with CRC32 instead. So that would make sense. > But one could broaden the scope a bit and make the argument that this > general idea of accepting some bytes, doing some "pure" arithmetic > operations from a particular discipline on them in slightly different > ways depending on the architecture, and then returning some other > bytes, is what in essence is happening with these all of these > function calls, no matter their security or intended use; so if crc32 > would benefit from being implemented using the exact same design > patterns, then it might as well be grouped in with the rest of Zinc. You should avoid suggesting it like this because you may end up implementing addition :-) You should stick to the initial intent which is to implement painful and boring stuff like crypto. But when you see how to implement CRC32, you'll see that it's as painful to implement as a crypto function. You start with a reference, you're not satisfied and start to optimize various parts of it until you break it for some inputs without noticing. I'm pretty sure that it reminds you your own experience with some crypto functions because I think we've all been in that situation in this domain. > On the other hand, this might be a bit of a slippery slope ("are > compression functions next?"), I was thinking about it as well. There's a relation between crypto and compression, all transform intelligible information into hidden one in a reversable way, so the API is quite similar. One could argue that compression usually requires memory allocation and huge states, so that's probably not for the same use cases if the functions have to deal with memory allocations. In my opinion currently we have : - asymmetric crypto whose goal is to reversably encrypt/decrypt in order to protect confidentiality on small data like keys, or to sign in order to authenticate small data like hashes - symmetric crypto whose goal is to protect confidentiality on data blocks. - hashes whose goal is to provide integrity of data blocks (crc32 fall into this category by the way) It's visible that despite using similar APIs, compression doesn't fit into any of these categories. > and a libcrc32.ko could just as well > exist as a standalone thing. Having it included could simplify exposing it as a static inline function when it's a single native instruction on the CPU. Just my two cents, Willy
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> If an implementation enters Zinc, it will go through my tree. If it >> enters the crypto API, it will go through Herbert's tree. If there >> wind up being messy tree dependency and merge timing issues, I can >> work this out in the usual way with Herbert. I'll be sure to discuss >> these edge cases with him when we discuss. I think there's a way to >> handle that with minimum friction. > > I would also strongly prefer that all crypto work is taken through > Herbert's tree, so we have a coherent view of it before it goes > upstream. I agree. I don't have any problems with the zinc code living in its own git tree. But any upstream merges should definitely go through the crypto tree because the inherent ties between the two code-base. Thanks,
Hi Herbert, On Tue, Oct 2, 2018 at 5:39 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > I would also strongly prefer that all crypto work is taken through > > Herbert's tree, so we have a coherent view of it before it goes > > upstream. > > I agree. I don't have any problems with the zinc code living in > its own git tree. But any upstream merges should definitely go > through the crypto tree because the inherent ties between the two > code-base. I can send you pull requests then if there are development cycles when there are in fact relations between the two trees. I'll update the commit message describing Zinc to include this. Jason
Hi Jason: On Tue, Oct 02, 2018 at 05:45:20AM +0200, Jason A. Donenfeld wrote: > Hi Herbert, > > > I agree. I don't have any problems with the zinc code living in > > its own git tree. But any upstream merges should definitely go > > through the crypto tree because the inherent ties between the two > > code-base. > > I can send you pull requests then if there are development cycles when > there are in fact relations between the two trees. I'll update the > commit message describing Zinc to include this. I think all merges should go through the crypto tree so that the kernel crypto community has oversight over the crypto implementations. Cheers,
On 2 October 2018 at 05:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hi Herbert, > > On Tue, Oct 2, 2018 at 5:39 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: >> > I would also strongly prefer that all crypto work is taken through >> > Herbert's tree, so we have a coherent view of it before it goes >> > upstream. >> >> I agree. I don't have any problems with the zinc code living in >> its own git tree. But any upstream merges should definitely go >> through the crypto tree because the inherent ties between the two >> code-base. > > I can send you pull requests then if there are development cycles when > there are in fact relations between the two trees. I'll update the > commit message describing Zinc to include this. > Can you explain why you it is so important to you that your changes remain outside the crypto tree? Also, I still think the name Zinc (zinc is not crypto/) is needlessly divisive and condescending, and unsaying it (in v2 and up) doesn't really work on the Internet (especially since you are still repeating it in your conference talk.)
On Tue, Oct 2, 2018 at 8:04 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 2 October 2018 at 05:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Herbert, > > > > On Tue, Oct 2, 2018 at 5:39 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > >> > I would also strongly prefer that all crypto work is taken through > >> > Herbert's tree, so we have a coherent view of it before it goes > >> > upstream. > >> > >> I agree. I don't have any problems with the zinc code living in > >> its own git tree. But any upstream merges should definitely go > >> through the crypto tree because the inherent ties between the two > >> code-base. > > > > I can send you pull requests then if there are development cycles when > > there are in fact relations between the two trees. I'll update the > > commit message describing Zinc to include this. > > > > Can you explain why you it is so important to you that your changes > remain outside the crypto tree? > > Also, I still think the name Zinc (zinc is not crypto/) is needlessly > divisive and condescending, and unsaying it (in v2 and up) doesn't > really work on the Internet (especially since you are still repeating > it in your conference talk.) I've been following the drama^Wdiscussion on the zinc for a long time now and I also think that "zinc" is a misleading name. Jason, you seem to hate the existing crypto framework with passion, and the name reflects that. Since we all agree that the framework can do better and your patches actually make it better, please just rename it to something that reflects what it is, a base framework. I think Ard already suggested "crypto/base/" or "crypto/core/".
On Tue, Oct 2, 2018 at 8:04 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Also, I still think the name Zinc (zinc is not crypto/) is needlessly > divisive and condescending, and unsaying it (in v2 and up) doesn't > really work on the Internet (especially since you are still repeating > it in your conference talk.) > Jason, you seem to hate the existing crypto framework with passion, > and the name reflects that. It's not divisive or condescending. I don't hate the existing framework with a passion -- this is patently false. The name even with its original acronym doesn't imply anything essentially negative. I see that you've repeatedly misinterpreted it this way -- which is why I removed that from v2 for the avoidance of doubt -- but that doesn't change the fact that its proper interpretation is not a condescending or divisive one. Look, people love to bikeshed about names. I'm sure you'll be able to CC-in a large crew of people who have opinions in one way or another, and this thread could begin to have many voices on that front or on multiple fronts. There are real benefits of sticking with the name I've given to the library I've spent enormous amounts of time writing. And so I'm going to stick with Zinc, since that's why our library is called. Sorry. We can talk about this at Plumbers in Vancouver if you want, but I think you're not going to get very far with a mailing list naming bikeshed. Meanwhile, I'm working around the clock to integrate your very useful technical suggestions, so please keep them coming if you still have technical nits you'd like to be in v7 of this patchset. Jason
On Tue, Oct 02, 2018 at 02:22:47PM +0200, Jason A. Donenfeld wrote: > On Tue, Oct 2, 2018 at 8:04 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Also, I still think the name Zinc (zinc is not crypto/) is needlessly > > divisive and condescending, and unsaying it (in v2 and up) doesn't > > really work on the Internet (especially since you are still repeating > > it in your conference talk.) > > Jason, you seem to hate the existing crypto framework with passion, > > and the name reflects that. > > It's not divisive or condescending. I don't hate the existing > framework with a passion -- this is patently false. The name even with > its original acronym doesn't imply anything essentially negative. I > see that you've repeatedly misinterpreted it this way -- which is why > I removed that from v2 for the avoidance of doubt -- but that doesn't > change the fact that its proper interpretation is not a condescending > or divisive one. > > Look, people love to bikeshed about names. I'm sure you'll be able to > CC-in a large crew of people who have opinions in one way or another, > and this thread could begin to have many voices on that front or on > multiple fronts. There are real benefits of sticking with the name > I've given to the library I've spent enormous amounts of time writing. > And so I'm going to stick with Zinc, since that's why our library is > called. Sorry. We can talk about this at Plumbers in Vancouver if you > want, but I think you're not going to get very far with a mailing list > naming bikeshed. > It's not really about the name, though. It's actually about the whole way of thinking about the submission. Is it a new special library with its own things going on, or is it just some crypto helper functions? It's really just the latter, but you've been presenting it as the former, which is causing a lot of unnecessary confusion and controversy. And I think that largely because you've started from that perspective, you've ended up with some oddities like your own directories in lib/ and include/ separate from the other existing crypto helper functions, and proposals to maintain it completely separately from the existing crypto code including having a separate git tree and separate upstreaming path. For example, we already have include/crypto/. What's the difference between include/crypto/ and include/zinc/? How do users know whether to include e.g. <crypto/chacha20.h> rather than <zinc/chacha20.h>? And are users going to remember and understand that "zinc" actually means "crypto" but not really? It would be much more straightforward and intuitive if we just had <crypto/chacha20.h>, and your new helper functions were declared in there. Dismissing these concerns as bikeshedding about the name is missing the point. - Eric
Hi Eric, On Wed, Oct 3, 2018 at 8:49 AM Eric Biggers <ebiggers@kernel.org> wrote: > It's not really about the name, though. It's actually about the whole way of > thinking about the submission. Is it a new special library with its own things > going on, or is it just some crypto helper functions? It's really just the > latter, but you've been presenting it as the former No, it really is its own thing with important differences from the present crypto api. Zinc's focus is on simplicity and clarity. To the extent that we're at all tangled with the current crypto api, the goal is to untangle as much as possible. It intends to be a small and lightweight set of routines, whose relationships are obvious, and with this direct and to the point organization, as well as work with the larger cryptography community and with academia to invite collaboration. With this comes a different way of maintaining it, with higher standards and a preference for different implementations than the current situation. With Zinc, you have an obvious series of C function calls composing the whole thing, without complicated indirection. It's something that could be trivially lifted out into a userspace library, and used broadly, for example -- something I'll probably do at some point. That's a bit of a design change to the current crypto api, and sprinkling some direct function calls within the current crypto api's complicated enterprise situation would only kick the can further down the road, as much complexity would still remain. The goal is to move away from behemoth enterprise APIs, and large and complex codebases to a simple and direct way of doing things. This desire to untangle, to start from a simpler base, and to generally do things differently means it will go into lib/zinc/ and include/zinc/ and have different maintainers. Jason
On Fri, Oct 5, 2018 at 3:14 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > On Wed, Oct 3, 2018 at 8:49 AM Eric Biggers <ebiggers@kernel.org> wrote: > > It's not really about the name, though. It's actually about the whole way of > > thinking about the submission. Is it a new special library with its own things > > going on, or is it just some crypto helper functions? It's really just the > > latter, but you've been presenting it as the former > > No, it really is its own thing with important differences from the > present crypto api. Zinc's focus is on simplicity and clarity. To the > extent that we're at all tangled with the current crypto api, the goal > is to untangle as much as possible. It intends to be a small and > lightweight set of routines, whose relationships are obvious, and with > this direct and to the point organization, as well as work with the larger > cryptography community and with academia to invite collaboration. With > this comes a different way of maintaining it, with higher standards > and a preference for different implementations than the current > situation. With Zinc, you have an obvious series of C function calls > composing the whole thing, without complicated indirection. It's > something that could be trivially lifted out into a userspace library, > and used broadly, for example -- something I'll probably do at some > point. That's a bit of a design change to the current crypto api, and > sprinkling some direct function calls within the current crypto api's > complicated enterprise situation would only kick the can further down > the road, as much complexity would still remain. The goal is to move > away from behemoth enterprise APIs, and large and complex codebases to > a simple and direct way of doing things. This desire to untangle, to > start from a simpler base, and to generally do things differently > means it will go into lib/zinc/ and include/zinc/ and have different > maintainers. So we will have two competing crypo stacks in the kernel? Having a lightweight crypto API is a good thing but I really don't like the idea of having zinc parallel to the existing crypto stack. And I strongly vote that Herbert Xu shall remain the maintainer of the whole crypto system (including zinc!) in the kernel.
On Fri, Oct 5, 2018 at 3:38 PM Richard Weinberger <richard.weinberger@gmail.com> wrote: > So we will have two competing crypo stacks in the kernel? > Having a lightweight crypto API is a good thing but I really don't like the idea > of having zinc parallel to the existing crypto stack. No, as you've seen in this patchset, the dynamic dispatch crypto API can trivially be done on top of Zinc. So each time we introduce a new primitive to Zinc that's also in the dynamic dispatch API, we reimplement the current crypto API in terms of Zinc. Check out the two patches in this series that do this; it's quite clean and sleek. > And I strongly vote that Herbert Xu shall remain the maintainer of the whole > crypto system (including zinc!) in the kernel. No, sorry, we intend to maintain the code we've written. But I am amenable to taking a tree-route into upstream based on whatever makes most sense with merge conflicts and such.
Am Freitag, 5. Oktober 2018, 15:46:29 CEST schrieb Jason A. Donenfeld: > On Fri, Oct 5, 2018 at 3:38 PM Richard Weinberger > <richard.weinberger@gmail.com> wrote: > > So we will have two competing crypo stacks in the kernel? > > Having a lightweight crypto API is a good thing but I really don't like the idea > > of having zinc parallel to the existing crypto stack. > > No, as you've seen in this patchset, the dynamic dispatch crypto API > can trivially be done on top of Zinc. So each time we introduce a new > primitive to Zinc that's also in the dynamic dispatch API, we > reimplement the current crypto API in terms of Zinc. Check out the two > patches in this series that do this; it's quite clean and sleek. This is why I was asking. Your statement and the code didn't match for me. > > And I strongly vote that Herbert Xu shall remain the maintainer of the whole > > crypto system (including zinc!) in the kernel. > > No, sorry, we intend to maintain the code we've written. But I am > amenable to taking a tree-route into upstream based on whatever makes > most sense with merge conflicts and such. So, you will be a sub-maintainer below Herbert's crypto, that's fine. What you wrote sounded like a parallel world... Thanks, //richard
From: Richard Weinberger <richard.weinberger@gmail.com> Date: Fri, 5 Oct 2018 15:37:57 +0200 > And I strongly vote that Herbert Xu shall remain the maintainer of > the whole crypto system (including zinc!) in the kernel. I 100% agree with this.
Changes v5->v6, along with who suggested it. -------------------------------------------- - [Eric Biggers] Cleaner and more efficient blake2s_final function. - [Eric Biggers] Remove modulo trick that was working around a gcc 8.1 bug. - [Eric Biggers] Use crypto_xor_cpy to avoid a memmove in chacha20. - In the unlikely condition that we transition from SIMD back to scalar instructions for Poly1305, it's important that we also convert back from base 2^26 to 2^64. This is super rare and weird, and it's hard to imagine somebody actually doing this for whatever bad reason, but it is a possibility, so we shouldn't calculate things wrong in that instance. - [Andrew Lunn] Change BUG_ON to WARN_ON in choice places. - [Thomas Gleixner] Explicitly dual license files under X where X≠GPL2 to be under X && GPL2. - [Andy Lutomirski] Use separate symbols for selftests that are of variable length, so that we neither waste space on disk nor waste space in memory (via __initconst). - [Thomas Gleixner] Make SPDX headers a standalone comment, and use the // commenting style in .h files, per the kernel style guide. - [Paul Burton] Allow assembler to fill branch delay slots on MIPS32r2, so that the implementation is more future-proof. - [Eric Biggers] Use Eric's scalar implementation for ChaCha20 ARM, which performs very well for Cortex-A7,5 and ARMv6, while using Andy Polyakov's NEON implementation for other ARMv7. - [Ard Biesheuvel] Reduce optimization from -O3 to -O2. - [Arn?d Bi?e(rgmann|shuvel)] Make sure stack frames stay inside 1024 bytes on 32-bit and 1280 bytes on 64-bit. - [Ard Biesheuvel] CRYPTOGAMS-related commit messages now have the corresponding OpenSSL commit written in them. - [Eric Biggers] Keep HChaCha20 key in native endian, since in some cases it can trivially be passed into the ChaCha20 block later. - Make constants follow a consistent naming scheme. - Workaround gcc 8 stack mis-optimization on m68k. - [Arnd Bergmann] Workaround gcc 8 stack mis-optimization with KASAN. Many of the above ARM changes are a result of discussions between Ard, Eric, AndyL, AndyP, Samuel, and me. These discussions are ongoing, and so it's possible we'll do another revision with further ARM tweaks. But perhaps this one will be sufficient for merging now, and we can continue to refine later in the cycle. ----------------------------------------------------------- This patchset is available on git.kernel.org in this branch, where it may be pulled directly for inclusion into net-next: * https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/log/?h=jd/wireguard ----------------------------------------------------------- WireGuard is a secure network tunnel written especially for Linux, which has faced around three years of serious development, deployment, and scrutiny. It delivers excellent performance and is extremely easy to use and configure. It has been designed with the primary goal of being both easy to audit by virtue of being small and highly secure from a cryptography and systems security perspective. WireGuard is used by some massive companies pushing enormous amounts of traffic, and likely already today you've consumed bytes that at some point transited through a WireGuard tunnel. Even as an out-of-tree module, WireGuard has been integrated into various userspace tools, Linux distributions, mobile phones, and data centers. There are ports in several languages to several operating systems, and even commercial hardware and services sold integrating WireGuard. It is time, therefore, for WireGuard to be properly integrated into Linux. Ample information, including documentation, installation instructions, and project details, is available at: * https://www.wireguard.com/ * https://www.wireguard.com/papers/wireguard.pdf As it is currently an out-of-tree module, it lives in its own git repo and has its own mailing list, and every commit for the module is tested against every stable kernel since 3.10 on a variety of architectures using an extensive test suite: * https://git.zx2c4.com/WireGuard https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/WireGuard.git/ * https://lists.zx2c4.com/mailman/listinfo/wireguard * https://www.wireguard.com/build-status/ The project has been broadly discussed at conferences, and was presented to the Netdev developers in Seoul last November, where a paper was released detailing some interesting aspects of the project. Dave asked me after the talk if I would consider sending in a v1 "sooner rather than later", hence this patchset. A decision is still waiting from the Linux Plumbers Conference, but an update on these topics may be presented in Vancouver in a few months. Prior presentations: * https://www.wireguard.com/presentations/ * https://www.wireguard.com/papers/wireguard-netdev22.pdf The cryptography in the protocol itself has been formally verified by several independent academic teams with positive results, and I know of two additional efforts on their way to further corroborate those findings. The version 1 protocol is "complete", and so the purpose of this review is to assess the implementation of the protocol. However, it still may be of interest to know that the thing you're reviewing uses a protocol with various nice security properties: * https://www.wireguard.com/formal-verification/ This patchset is divided into four segments. The first introduces a very simple helper for working with the FPU state for the purposes of amortizing SIMD operations. The second segment is a small collection of cryptographic primitives, split up into several commits by primitive and by hardware. The third shows usage of Zinc within the existing crypto API and as a replacement to the existing crypto API. The last is WireGuard itself, presented as an unintrusive and self-contained virtual network driver. It is intended that this entire patch series enter the kernel through DaveM's net-next tree. Subsequently, WireGuard patches will go through DaveM's net-next tree, while Zinc patches will go through Greg KH's tree. Enjoy, Jason