[GIT,PULL] Asymmetric keys fix for v6.4-rc5

Hi Linus

sorry for this unusual procedure of me requesting a patch to be pulled.
I asked for several months the maintainers (David: asymmetric keys,
Jarkko: key subsystem) to pick my patch but without any luck.

I signed the tag, but probably it would not matter, since my key is not
among your trusted keys.

The following changes since commit 921bdc72a0d68977092d6a64855a1b8967acc1d9:

  Merge tag 'mmc-v6.4-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc (2023-06-02 08:35:13 -0400)

are available in the Git repository at:

  https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5

for you to fetch changes up to c3d03e8e35e005e1a614e51bb59053eeb5857f76:

  KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() (2023-06-02 15:36:23 +0200)

----------------------------------------------------------------
Asymmetric keys fix for v6.4-rc5

Here is a small fix to make an unconditional copy of the buffer passed
to crypto operations, to take into account the case of the stack not in
the linear mapping area.

It has been tested and verified to fix the bug.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

----------------------------------------------------------------
Roberto Sassu (1):
      KEYS: asymmetric: Copy sig and digest in public_key_verify_signature()

 crypto/asymmetric_keys/public_key.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

On Sat, Jun 03, 2023 at 12:41:00PM +0200, Roberto Sassu wrote:
> On 6/3/2023 2:02 AM, Linus Torvalds wrote:
> > On Fri, Jun 2, 2023 at 1:38 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > 
> > > The patch re-uses the allocation it already does for the key data, and
> > > it seems sane.
> > 
> > Ugh. I had to check that it was ok to re-use the key buffer, but it
> > does seem to be the case that you can just re-use the buffer after
> > you've done that crypto_akcipher_set_priv/pub_key() call, and the
> > crypto layer has to copy it into its own data structures.
> 
> Yes, we could not do it if the set_pub_key/set_priv_key methods use
> internally the passed pointer. I guess it depends on the methods, for RSA
> and ECDSA it seems fine (they copy to a different location).
> 
> The doubt comes because the buffer is freed after crypto_wait_req() and not
> after crypto_akcipher_set_*_key(), suggesting that it could be actually used
> during the crypto operation.
> 
> Rechecked the thread, and the suggestion to reuse the buffer and not append
> the signature and digest at the end was by Eric Biggers.
> 
> Eric, in light of this finding, should we still reuse the buffer?
> 

I don't think there was any "finding" here.  The setkey methods in the crypto
API aren't allowed to reuse the buffer they are passed, so the patch is fine.

- Eric

On Fri, Jun 02, 2023 at 08:02:23PM -0400, Linus Torvalds wrote:
>
> I absolutely abhor the crypto interfaces. They all seem designed for
> that "external DMA engine" case that seems so horrendously pointless
> and slow.  In practice so few of them are that, and we have all those
> optimized routines for doing it all on the CPU - but have in the
> meantime wasted all that time and effort into copying everything,
> turning simple buffers into sg-bufs etc etc. The amount of indirection
> and "set this state in the state machine" is just nasty, and this
> seems to all be a prime example of it all. With some of it then
> randomly going through some kthread too.

You're right.  Originally SG lists were used as the majority of
our input came from network packets, in the form of skb's.  They
are easily translated into SG lists.  This is still somewhat the
case for parts of the Crypto API (e.g., skcipher and ahash).

However, for akcipher the only user of the underlying API is the
file in question so I absolutely agree that forcing it to go through
an SG list is just wrong.

I'll change the underlying akcipher interface to take pointers
instead and hide the SG list stuff (along with the copying) inside
API.

In the mean time feel free to take this patch as it appears to be
correct and should keep things chugging along while we work on the
API.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,

Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:

> Here is a small fix to make an unconditional copy of the buffer passed
> to crypto operations, to take into account the case of the stack not in
> the linear mapping area.

I wonder if evm_verify_hmac() and other such callers of the signature
verification service should be placing the data and crypto material in slab
memory rather than it being on the stack.  But, for the moment:

Acked-by: David Howells <dhowells@redhat.com>

The pull request you sent on Fri, 02 Jun 2023 16:41:04 +0200:

> https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f8dba31b0a826e691949cd4fdfa5c30defaac8c5

Thank you!

On Mon, 5 Jun 2023 at 10:49, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, Jun 02, 2023 at 08:02:23PM -0400, Linus Torvalds wrote:
> >
> > I absolutely abhor the crypto interfaces. They all seem designed for
> > that "external DMA engine" case that seems so horrendously pointless
> > and slow.  In practice so few of them are that, and we have all those
> > optimized routines for doing it all on the CPU - but have in the
> > meantime wasted all that time and effort into copying everything,
> > turning simple buffers into sg-bufs etc etc. The amount of indirection
> > and "set this state in the state machine" is just nasty, and this
> > seems to all be a prime example of it all. With some of it then
> > randomly going through some kthread too.
>
> You're right.  Originally SG lists were used as the majority of
> our input came from network packets, in the form of skb's.  They
> are easily translated into SG lists.  This is still somewhat the
> case for parts of the Crypto API (e.g., skcipher and ahash).
>
> However, for akcipher the only user of the underlying API is the
> file in question so I absolutely agree that forcing it to go through
> an SG list is just wrong.
>
> I'll change the underlying akcipher interface to take pointers
> instead and hide the SG list stuff (along with the copying) inside
> API.
>

Could we do the same for the compression API? This is a major pain as
well, and results (on my 128-core workstation) in 32 MiB permanently
tied up in scratch buffers in the scomp-to-acomp adaptation layer
because most of the underlying implementations are compression
libraries operating on plain virtual addresses, and so the
scatterlists needs to be copied into a buffer and back to perform the
actual transformation.

The only user user of the async compression interface is zswap, but it
blocks on the completion so it is actually synchronous as well.