[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 Fri, Jun 2, 2023 at 10:41 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> 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.

Hmm.

The patch behind that tag looks sane to me, but this is not code I am
hugely familiar with.

Who is the caller that passes in the public_key_signature data on the
stack to public_key_verify_signature()? This may well be the right
point to move it away from the stack in order to have a valid sg-list,
but even if this patch is all good, it would be nice to have the call
chain documented as part of the commit message.

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

It does matter - I do pull from people even without full chains, I
just end up being a lot more careful, and I still want to see the
signature for any future reference...

DavidH, Herbert, please comment:

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

basically public_key_verify_signature() is passed that

     const struct public_key_signature *sig

as an argument, and currently does

        sg_init_table(src_sg, 2);
        sg_set_buf(&src_sg[0], sig->s, sig->s_size);
        sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);


on it which is *not* ok if the s->s and s->digest points to stack data
that ends up not dma'able because of a virtually mapped stack.

The patch re-uses the allocation it already does for the key data, and
it seems sane.

But again, this is not code I look at normally, so...

               Linus
  

On Fri, 2023-06-02 at 13:38 -0400, Linus Torvalds wrote:
> On Fri, Jun 2, 2023 at 10:41 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > 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.
> 
> Hmm.
> 
> The patch behind that tag looks sane to me, but this is not code I am
> hugely familiar with.
> 
> Who is the caller that passes in the public_key_signature data on the
> stack to public_key_verify_signature()? This may well be the right
> point to move it away from the stack in order to have a valid sg-list,
> but even if this patch is all good, it would be nice to have the call
> chain documented as part of the commit message.

Oh, it seems it was only in the first version of the patch:

https://lore.kernel.org/linux-kernel/20221104122023.1750333-1-roberto.sassu@huaweicloud.com/

Originally, the kernel panic was due to EVM, but I later found that IMA
Appraisal could have caused the same.

> > I signed the tag, but probably it would not matter, since my key is not
> > among your trusted keys.
> 
> It does matter - I do pull from people even without full chains, I
> just end up being a lot more careful, and I still want to see the
> signature for any future reference...

Ok, then it makes sense to push my key to a key server.

Thanks

Roberto

> DavidH, Herbert, please comment:
> 
> >   https://github.com/robertosassu/linux.git tags/asym-keys-fix-for-linus-v6.4-rc5
> 
> basically public_key_verify_signature() is passed that
> 
>      const struct public_key_signature *sig
> 
> as an argument, and currently does
> 
>         sg_init_table(src_sg, 2);
>         sg_set_buf(&src_sg[0], sig->s, sig->s_size);
>         sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
> 
> 
> on it which is *not* ok if the s->s and s->digest points to stack data
> that ends up not dma'able because of a virtually mapped stack.
> 
> The patch re-uses the allocation it already does for the key data, and
> it seems sane.
> 
> But again, this is not code I look at normally, so...
> 
>                Linus
  

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.

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.

I still think that patch is probably fine, but was also going "maybe
the real problem is in that library helper function
(asymmetric_verify(), in this case), which takes those (sig, siglen,
digest, digestlen) arguments and turns it into a 'struct
public_key_signature' without marshalling them.

Just looking at this mess of indirection and different "helper"
functions makes me second-guess myself about where the actual
conversion should be - while also feeling like it should never have
been done as a scatter-gather entry in the first place.

Anyway, I don't feel competent to decide if that pull request is the
right fix or not.

But it clearly is *a* fix.

            Linus
  

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?

Thanks

Roberto

> 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.
> 
> I still think that patch is probably fine, but was also going "maybe
> the real problem is in that library helper function
> (asymmetric_verify(), in this case), which takes those (sig, siglen,
> digest, digestlen) arguments and turns it into a 'struct
> public_key_signature' without marshalling them.
> 
> Just looking at this mess of indirection and different "helper"
> functions makes me second-guess myself about where the actual
> conversion should be - while also feeling like it should never have
> been done as a scatter-gather entry in the first place.
> 
> Anyway, I don't feel competent to decide if that pull request is the
> right fix or not.
> 
> But it clearly is *a* fix.
> 
>              Linus
  

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 6/3/2023 6:02 PM, Eric Biggers wrote:
> 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.

That was the information I was missing.

Thanks!

Roberto
  

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.