Re: [PATCH 2/5] crypto: akcipher - Drop usage of sglists for verify op

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 29 Jul 2024 15:48:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Commit 6cb8815f41a9 ("crypto: sig - Add interface for sign/verify")
> introduced an API which accepts kernel buffers instead of sglists for
> signature generation and verification.
> 
> Commit 63ba4d67594a ("KEYS: asymmetric: Use new crypto interface without
> scatterlists") converted the sole user in the tree to the new API.
> 
> Although the API externally accepts kernel buffers, internally it still
> converts them to sglists, which results in overhead for asymmetric
> algorithms because they need to copy the sglists back into kernel
> buffers.
> 
> Take the next step and switch signature verification over to using
> kernel buffers internally, thereby avoiding the sglists overhead.
> 
> Because all ->verify implementations are synchronous, forego invocation
> of crypto_akcipher_sync_{prep,post}() and call crypto_akcipher_verify()
> directly from crypto_sig_verify().
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

I'm a little out of my depth in this code, but one question did
come to mind. Rather than passing scatter lists and buffers
via the same function void akcipher_request_set_crypt()
why not add a variant that takes buffers for signing cases so
that you can maintain the scatterlist pointers for the encrypt/decrypt
cases?



> diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
> index 18a10cad07aa..2c5bc35d297a 100644
> --- a/include/crypto/akcipher.h
> +++ b/include/crypto/akcipher.h
> @@ -16,28 +16,39 @@
>   *
>   * @base:	Common attributes for async crypto requests
>   * @src:	Source data
> - *		For verify op this is signature + digest, in that case
> - *		total size of @src is @src_len + @dst_len.
> - * @dst:	Destination data (Should be NULL for verify op)
> + * @dst:	Destination data
>   * @src_len:	Size of the input buffer
> - *		For verify op it's size of signature part of @src, this part
> - *		is supposed to be operated by cipher.
> - * @dst_len:	Size of @dst buffer (for all ops except verify).
> + * @dst_len:	Size of @dst buffer
>   *		It needs to be at least	as big as the expected result
>   *		depending on the operation.
>   *		After operation it will be updated with the actual size of the
>   *		result.
>   *		In case of error where the dst sgl size was insufficient,
>   *		it will be updated to the size required for the operation.
> - *		For verify op this is size of digest part in @src.
> + * @sig:	Signature
> + * @digest:	Digest
> + * @sig_len:	Size of @sig
> + * @digest_len:	Size of @digest
>   * @__ctx:	Start of private context data
>   */
>  struct akcipher_request {
>  	struct crypto_async_request base;
> -	struct scatterlist *src;
> -	struct scatterlist *dst;
> -	unsigned int src_len;
> -	unsigned int dst_len;
> +	union {
> +		struct {
> +			/* sign, encrypt, decrypt operations */
> +			struct scatterlist *src;
> +			struct scatterlist *dst;
> +			unsigned int src_len;
> +			unsigned int dst_len;
> +		};
> +		struct {
> +			/* verify operation */
> +			const void *sig;
> +			const void *digest;
> +			unsigned int sig_len;
> +			unsigned int digest_len;
> +		};
> +	};
>  	void *__ctx[] CRYPTO_MINALIGN_ATTR;
>  };
>  
> @@ -242,20 +253,18 @@ static inline void akcipher_request_set_callback(struct akcipher_request *req,
>   * Sets parameters required by crypto operation
>   *
>   * @req:	public key request
> - * @src:	ptr to input scatter list
> - * @dst:	ptr to output scatter list or NULL for verify op
> - * @src_len:	size of the src input scatter list to be processed
> - * @dst_len:	size of the dst output scatter list or size of signature
> - *		portion in @src for verify op
> + * @src:	ptr to input scatter list or signature for verify op
> + * @dst:	ptr to output scatter list or digest for verify op
> + * @src_len:	size of @src
> + * @dst_len:	size of @dst
>   */
>  static inline void akcipher_request_set_crypt(struct akcipher_request *req,
> -					      struct scatterlist *src,
> -					      struct scatterlist *dst,
> +					      const void *src, const void *dst,
Maybe it's worth a 'special' variant.
static inline void akcipher_request_set_crypt_for_verify()
so that you can keep the other list typed and not rely
on overlapping pointer fields via the union.

>  					      unsigned int src_len,
>  					      unsigned int dst_len)
>  {
> -	req->src = src;
> -	req->dst = dst;
> +	req->sig = src;
> +	req->digest = dst;
>  	req->src_len = src_len;
>  	req->dst_len = dst_len;
>  }
> @@ -372,10 +381,6 @@ static inline int crypto_akcipher_sign(struct akcipher_request *req)
>   *
>   * @req:	asymmetric key request
>   *
> - * Note: req->dst should be NULL, req->src should point to SG of size
> - * (req->src_size + req->dst_size), containing signature (of req->src_size
> - * length) with appended digest (of req->dst_size length).
> - *
>   * Return: zero on verification success; error code in case of error.
>   */
>  static inline int crypto_akcipher_verify(struct akcipher_request *req)