Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <97fd9066-9afc-9faa-a604-46110ed1268c@huaweicloud.com>
Date:   Sat, 3 Jun 2023 12:41:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [GIT PULL] Asymmetric keys fix for v6.4-rc5
Content-Language: en-US
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        David Howells <dhowells@redhat.com>,
        Herbert Xu <herbert@gondor.apana.org.au>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Eric Biggers <ebiggers@kernel.org>,
        Stefan Berger <stefanb@linux.ibm.com>, davem@davemloft.net,
        zohar@linux.ibm.com, dmitry.kasatkin@gmail.com,
        paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com,
        Jarkko Sakkinen <jarkko@kernel.org>,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <4d7e38ff5bbc496cb794b50e1c5c83bcd2317e69.camel@huaweicloud.com>
 <CAHk-=wj4S0t5RnJQmF_wYwv+oMTKggwdLnrA9D1uMNKq4H4byw@mail.gmail.com>
 <CAHk-=wgCUzRNTg4fC8DF=UFnznK0M=mNUBDcsnLt7D4+HP2_1Q@mail.gmail.com>
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
In-Reply-To: <CAHk-=wgCUzRNTg4fC8DF=UFnznK0M=mNUBDcsnLt7D4+HP2_1Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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