Message-ID: <CAMj1kXGxxRs6Rkhevm9NSY6TaJUsOmF3UqdHUo=NRg9kQKtSBA@mail.gmail.com>
Date: Fri, 15 Mar 2024 00:38:45 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: James Prestwood <prestwoj@...il.com>, Jeff Johnson <quic_jjohnson@...cinc.com>, 
	Johannes Berg <johannes@...solutions.net>, Karel Balej <balejk@...fyz.cz>, 
	dimitri.ledkov@...onical.com, alexandre.torgue@...s.st.com, 
	davem@...emloft.net, dhowells@...hat.com, herbert@...dor.apana.org.au, 
	keyrings@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	mcgrof@...nel.org, mcoquelin.stm32@...il.com, linux-wireless@...r.kernel.org, 
	netdev@...r.kernel.org, iwd@...ts.linux.dev
Subject: Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

On Thu, 14 Mar 2024 at 21:20, Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> > IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> > Anything that wifi requires as far as crypto goes IWD uses the kernel,
> > except ECC is the only exception. The entire list of crypto requirements
> > (for full support at least) for IWD is here:
> >
> > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config
>
> That's quite an extensive list, and it's not documented in the iwd README.
> Don't you get bug reports from users who are running a kernel that's missing one
> of those options?
>
> > For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> > operations, (query), encrypt, decrypt, sign, verify.
> >
> > I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> > I started working on IWD so I was not aware the documentation was so poor.
> > That is an entirely separate issue than this IMO, and I'm happy to help with
> > getting docs updated to include a proper list of supported features. In
> > addition maybe some automated testing that gets run on kernel builds which
> > actually exercises this API so it doesn't get accidentally get broken in the
> > future? Docs/tests IMO are the proper "fix" here, not telling someone to
> > stop using an API that has existed a long time.
>
> I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
> collaboration between the iwd developers and the kernel keyrings maintainer.
> So, as far as I can tell, it's not that the kernel had an existing API that iwd
> started using.  It's that iwd got some APIs added to the kernel for themselves.
> KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
> doesn't return any notable results.  keyctl does provide a command-line
> interface to them, but I can't find any users of the keyctl commands either.
>
> Then, everyone disappeared and it got dumped on the next generation of kernel
> developers, who often don't know that this API even exists.  And since the API
> is also poorly specified and difficult to maintain (e.g., changing a seemingly
> unrelated part of the kernel can break it), the results are predictable...  And
> of course the only thing that breaks is iwd, since it's the only user.
>
> It would be worth taking a step back and looking at the overall system
> architecture here.  Is this the best way to ensure a reliable wireless
> experience for Linux users?
>
> Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
> different direction (e.g. using OpenSSL) should be taken...
>
> (Another issue with the kernel keyrings stuff is that provides a significant
> attack surface for the kernel to be exploited.)
>
> If you do decide to continue with the status quo, it may be necessary for the
> iwd developers to take a more active role in maintaining this API in order to
> ensure it continues working properly for you.
>
> AF_ALG is on *slightly* firmer ground since it's been around for longer, is
> properly part of the crypto subsystem, and has a few other users.  Unfortunately
> it still suffers from the same issues though, just to a slightly lesser degree.
>

We dropped MD4 because there are no users in the kernel. It is not the
kernel's job to run code on behalf of user space if it does not
require any privileges and can therefore execute in user space
directly.

The fact that AF_ALG permits this is a huge oversight on the part of
the kernel community, and a major maintenance burden. The point of
AF_ALG was to expose hardware crypto accelerators (which are shared
resources that /need/ to be managed by the kernel) to user space, and
we inadvertently ended up allowing the kernel's pure-software
algorithms to be used in the same way.

The fact that we even added APIs to the kernel to accommodate iwd is
even worse. It means system call overhead (which has become worse due
to all the speculation mitigations) to execute some code that could
execute in user space just as well, which is a bad idea for other
reasons too (for instance, accelerated crypto that uses SIMD in the
kernel disables preemption on many architectures, resulting in
scheduling jitter)

Note that in the case of iwd, it is unlikely that the use of AF_ALG
could ever result in meaningful use of hardware accelerators: today's
wireless interfaces don't use software crypto for the bulk of the data
(i.e., the packets themselves) and the wireless key exchange protocols
etc are unlikely to be supported in generic crypto accelerators, and
even if they were, the latency would likely result in worse
performance overall than a software implementation.

So iwd's deliberate choice to use the kernel as a crypto library is
severely misguided. I have made the same point 4 years ago when I
replaced iwd's use of the kernel's ecb(arc4) code with a suitable
software implementation (3 files changed, 53 insertions, 40
deletions). Of course, replacing other algorithms will take more work
than that, but it is the only sensible approach. We all know the cat
is out of the bag when it comes to AF_ALG, and we simply have to
retain all those broken algorithms as executable code at the kernel's
privileged execution level, just in case some user space is still
around that relies on it. But that doesn't mean we cannot be very
clear about our preferred way forward.

Powered by blists - more mailing lists