On Wed, Mar 13, 2024 at 04:06:11PM -0700, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote: > > On 3/13/2024 3:10 PM, Eric Biggers wrote: > > > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote: > > >> Hi, > > >> > > >> On 3/13/24 1:22 PM, Eric Biggers wrote: > > >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: > > >>>> Hi, > > >>>> > > >>>> On 3/13/24 12:44 PM, Eric Biggers wrote: > > >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: > > >>>>>> Hi, > > >>>>>> > > >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote: > > >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more > > >>>>>>> doesn't hurt ... > > >>>>>>> > > >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: > > >>>>>>>> and I use iwd > > >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any > > >>>>>>> kernel crypto code for 802.1X. > > >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what > > >>>>>> you meant by "problem". > > >>>>>> > > >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that > > >>>>>> is the problem. > > >>>>>> > > >>>>>> The original commit says it was to remove support for sha1 signed kernel > > >>>>>> modules, but it did more than that and broke the keyctl API. > > >>>>>> > > >>>>> Which specific API is iwd using that is relevant here? > > >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd > > >>>>> and grepped for keyctl and AF_ALG, but there are no matches. > > >>>> IWD uses ELL for its crypto, which uses the AF_ALG API: > > >>>> > > >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > >>> Thanks for pointing out that the relevant code is really in that separate > > >>> repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The > > >>> blamed commit didn't change anything for AF_ALG. > > >>> > > >>>> I believe the failure is when calling: > > >>>> > > >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1" > > >>>> > > >>>> From logs Michael posted on the IWD list, the ELL API that fails is: > > >>>> > > >>>> l_key_get_info (ell.git/ell/key.c:416) > > >>> Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a > > >>> weird set of APIs where userspace can ask the kernel to do asymmetric key > > >>> operations. It's unclear why they exist, as the same functionality is available > > >>> in userspace crypto libraries. > > >>> > > >>> I suppose that the blamed commit, or at least part of it, will need to be > > >>> reverted to keep these weird keyctls working. > > >>> > > >>> For the future, why doesn't iwd just use a userspace crypto library such as > > >>> OpenSSL? > > >> > > >> I was not around when the original decision was made, but a few reasons I > > >> know we don't use openSSL: > > >> > > >> - IWD has virtually zero dependencies. > > > > > > Depending on something in the kernel does not eliminate a dependency; it just > > > adds that particular kernel UAPI to your list of dependencies. The reason that > > > we're having this discussion in the first place is because iwd is depending on > > > an obscure kernel UAPI that is not well defined. Historically it's been hard to > > > avoid "breaking" changes in these crypto-related UAPIs because of the poor > > > design where a huge number of algorithms are potentially supported, but the list > > > is undocumented and it varies from one system to another based on configuration. > > > Also due to their obscurity many kernel developers don't know that these UAPIs > > > even exist. (The reaction when someone finds out is usually "Why!?") > > > > > > It may be worth looking at if iwd should make a different choice for this > > > dependency. It's understandable to blame dependencies when things go wrong, but > > > at the same time the choice of dependency is very much a choice, and some > > > choices can be more technically sound and cause fewer problems than others... > > > > > >> - OpenSSL + friends are rather large libraries. > > > > > > The Linux kernel is also large, and it's made larger by having to support > > > obsolete crypto algorithms for backwards compatibility with iwd. > > > > > >> - AF_ALG has transparent hardware acceleration (not sure if openSSL does > > >> too). > > > > > > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI. > > > > > >> Another consideration is once you support openSSL someone wants wolfSSL, > > >> then boringSSL etc. Even if users implement support it just becomes a huge > > >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/ > > >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is > > >> ~5k. You have to sort out all the nitty gritty details of each library, and > > >> provide a common driver/API for the core code, differences between openssl > > >> versions, the list goes on. > > > > > > What is the specific functionality that you're actually relying on that you > > > think would need 40K lines of code to replace, even using OpenSSL? I see you > > > are using KEYCTL_PKEY_*, but what specifically are you using them for? What > > > operations are being performed, and with which algorithms and key formats? > > > Also, is the kernel behavior that you're relying on documented anywhere? There > > > are man pages for those keyctls, but they don't say anything about any > > > particular hash algorithm, SHA-1 or otherwise, being supported. > > > > <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@xxxxxxxxxxxxxx/> > > "And we simply do not break user space." > > -Linus Torvalds > > > > Is this no longer applicable? > > > > As I said, the commit, or at least the part of it that broke iwd (it's not clear > that it's the whole commit), needs to be reverted. > > I just hope that, simultaneously, the iwd developers will consider improving the > design of iwd to avoid this type of recurring issue in the future. After all, > this may be the only real chance for such a discussion before the next time iwd > breaks. > > Also, part of the reason I'm asking about what functionality that iwd is relying > on is so that, if necessary, it can be properly documented and supported... > > If we don't know what we are supporting, it is very hard to support it. I ended up just sending out a patch that does a full revert: https://lore.kernel.org/linux-crypto/20240313233227.56391-1-ebiggers@xxxxxxxxxx Again, regardless of that, these issues with iwd have been recurring, and it is still worth discussing the best way from a technical perspective to prevent them from happening in the future... There's a fairly clear path to achieve that. - Eric
