[PATCH v2 00/19] Migrate to sig_alg and templatize ecdsa

[PATCH v2 00/19] Migrate to sig_alg and templatize ecdsa

[Lukas Wunner]

The original impetus of this series is to introduce P1363 signature
decoding for ecdsa (patch [18/19]), which is needed by the upcoming
SPDM library (Security Protocol and Data Model) for PCI device
authentication.

To facilitate that, move X9.62 signature decoding out of ecdsa.c and
into a template (patch [15/19]).

New in v2:  Move the maximum signature size calculations for ecdsa
out of software_key_query() and into the X9.62 template so that
corresponding calculations can be added for P1363 without further
cluttering up software_key_query() (patch [16/19] - [17/19]).

New in v2:  Avoid inefficient copying from kernel buffers to sglists
in the new templates by introducing a sig_alg backend and migrating
all algorithms to it, per Herbert's advice (patch [02/19] - [12/19]).

Clean up various smaller issues that caught my eye in ecdsa
(patch [01/19] and [14/19]), ecrdsa (patch [19/19]) and
ASN.1 headers (patch [13/19]).

I've also accumulated various cleanups for crypto virtio on my
development branch but will leave them for another day as this
series is already nearing the "too big to review" threshold. ;)

I've run selftests on every single commit, but further testing
would be appreciated to raise the confidence.


Link to v1:

https://lore.kernel.org/all/cover.1722260176.git.lukas@wunner.de/

Changes v1 -> v2:

* [PATCH 13/19] ASN.1: Clean up include statements in public headers
  * Drop "#include <linux/bug.h>" from <linux/asn1_encoder.h> (Jonathan)

* [PATCH 14/19] crypto: ecdsa - Avoid signed integer overflow on signature
  decoding
  * Add code comment explaining why vlen may be larger than bufsize (Stefan)

* [PATCH 15/19] crypto: ecdsa - Move X9.62 signature decoding into template
  * Drop unnecessary "params", "param_len" and "algo" definitions from
    ecdsa_nist_p{192,256,384,521}_tv_template[].
  * Introduce and use struct ecdsa_raw_sig in <crypto/internal/ecc.h>.

* [PATCH 18/19] crypto: ecdsa - Support P1363 signature decoding
  * Drop unnecessary "params", "param_len" and "algo" definitions from
    p1363_ecdsa_nist_p256_tv_template[].


Lukas Wunner (19):
  crypto: ecdsa - Drop unused test vector elements
  crypto: sig - Introduce sig_alg backend
  crypto: ecdsa - Migrate to sig_alg backend
  crypto: ecrdsa - Migrate to sig_alg backend
  crypto: rsa-pkcs1pad - Deduplicate set_{pub,priv}_key callbacks
  crypto: rsassa-pkcs1 - Migrate to sig_alg backend
  crypto: rsassa-pkcs1 - Harden digest length verification
  crypto: rsassa-pkcs1 - Avoid copying hash prefix
  crypto: virtio - Drop sign/verify operations
  crypto: drivers - Drop sign/verify operations
  crypto: akcipher - Drop sign/verify operations
  crypto: sig - Move crypto_sig_*() API calls to include file
  ASN.1: Clean up include statements in public headers
  crypto: ecdsa - Avoid signed integer overflow on signature decoding
  crypto: ecdsa - Move X9.62 signature decoding into template
  crypto: sig - Rename crypto_sig_maxsize() to crypto_sig_keysize()
  crypto: ecdsa - Move X9.62 signature size calculation into template
  crypto: ecdsa - Support P1363 signature decoding
  crypto: ecrdsa - Fix signature size calculation

 Documentation/crypto/api-akcipher.rst         |   2 +-
 Documentation/crypto/api-sig.rst              |  15 +
 Documentation/crypto/api.rst                  |   1 +
 Documentation/crypto/architecture.rst         |   2 +
 crypto/Kconfig                                |   5 +-
 crypto/Makefile                               |   5 +-
 crypto/akcipher.c                             |  64 +-
 crypto/asymmetric_keys/public_key.c           |  58 +-
 crypto/ecdsa-p1363.c                          | 159 ++++
 crypto/ecdsa-x962.c                           | 237 +++++
 crypto/ecdsa.c                                | 209 ++---
 crypto/ecrdsa.c                               |  64 +-
 crypto/internal.h                             |  19 -
 crypto/rsa-pkcs1pad.c                         | 371 +-------
 crypto/rsa.c                                  |  17 +-
 crypto/rsassa-pkcs1.c                         | 442 +++++++++
 crypto/sig.c                                  | 143 +--
 crypto/testmgr.c                              | 320 +++++--
 crypto/testmgr.h                              | 884 +++++++++++++++---
 drivers/crypto/aspeed/aspeed-acry.c           |   2 -
 drivers/crypto/hisilicon/hpre/hpre_crypto.c   |   2 -
 drivers/crypto/starfive/jh7110-rsa.c          |   2 -
 .../virtio/virtio_crypto_akcipher_algs.c      |  65 +-
 include/crypto/akcipher.h                     |  69 +-
 include/crypto/internal/akcipher.h            |   4 +-
 include/crypto/internal/ecc.h                 |  14 +
 include/crypto/internal/rsa.h                 |  29 +
 include/crypto/internal/sig.h                 |  80 ++
 include/crypto/sig.h                          | 152 ++-
 include/linux/asn1_decoder.h                  |   1 +
 include/linux/asn1_encoder.h                  |   1 -
 include/linux/slab.h                          |   1 +
 include/uapi/linux/cryptouser.h               |   5 +
 include/uapi/linux/virtio_crypto.h            |   1 +
 security/integrity/ima/ima_main.c             |   6 +-
 35 files changed, 2398 insertions(+), 1053 deletions(-)
 create mode 100644 Documentation/crypto/api-sig.rst
 create mode 100644 crypto/ecdsa-p1363.c
 create mode 100644 crypto/ecdsa-x962.c
 create mode 100644 crypto/rsassa-pkcs1.c

-- 
2.43.0

[PATCH v2 09/19] crypto: virtio - Drop sign/verify operations

[Lukas Wunner]

The virtio crypto driver exposes akcipher sign/verify operations in a
user space ABI.  This blocks removal of sign/verify from akcipher_alg.

Herbert opines:

   "I would say that this is something that we can break.  Breaking it
    is no different to running virtio on a host that does not support
    these algorithms.  After all, a software implementation must always
    be present.

    I deliberately left akcipher out of crypto_user because the API
    is still in flux.  We should not let virtio constrain ourselves."
    https://lore.kernel.org/all/ZtqoNAgcnXnrYhZZ@gondor.apana.org.au/

   "I would remove virtio akcipher support in its entirety.  This API
    was never meant to be exposed outside of the kernel."
    https://lore.kernel.org/all/Ztqql_gqgZiMW8zz@gondor.apana.org.au/

Drop sign/verify support from virtio crypto.  There's no strong reason
to also remove encrypt/decrypt support, so keep it.

A key selling point of virtio crypto is to allow guest access to crypto
accelerators on the host.  So far the only akcipher algorithm supported
by virtio crypto is RSA.  Dropping sign/verify merely means that the
PKCS#1 padding is now always generated or verified inside the guest,
but the actual signature generation/verification (which is an RSA
decrypt/encrypt operation) may still use an accelerator on the host.

Generating or verifying the PKCS#1 padding is cheap, so a hardware
accelerator won't be of much help there.  Which begs the question
whether virtio crypto support for sign/verify makes sense at all.

It would make sense for the sign operation if the host has a security
chip to store asymmetric private keys.  But the kernel doesn't even
have an asymmetric_key_subtype yet for hardware-based private keys.
There's at least one rudimentary driver for such chips (atmel-ecc.c for
ATECC508A), but it doesn't implement the sign operation.  The kernel
would first have to grow support for a hardware asymmetric_key_subtype
and at least one driver implementing the sign operation before exposure
to guests via virtio makes sense.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 .../virtio/virtio_crypto_akcipher_algs.c      | 65 ++++++-------------
 include/uapi/linux/virtio_crypto.h            |  1 +
 2 files changed, 22 insertions(+), 44 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index cb92b7fa99c6..48fee07b7e51 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -83,23 +83,16 @@ static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
 	case VIRTIO_CRYPTO_BADMSG:
 		error = -EBADMSG;
 		break;
-
-	case VIRTIO_CRYPTO_KEY_REJECTED:
-		error = -EKEYREJECTED;
-		break;
-
 	default:
 		error = -EIO;
 		break;
 	}
 
 	akcipher_req = vc_akcipher_req->akcipher_req;
-	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY) {
-		/* actuall length maybe less than dst buffer */
-		akcipher_req->dst_len = len - sizeof(vc_req->status);
-		sg_copy_from_buffer(akcipher_req->dst, sg_nents(akcipher_req->dst),
-				    vc_akcipher_req->dst_buf, akcipher_req->dst_len);
-	}
+	/* actual length maybe less than dst buffer */
+	akcipher_req->dst_len = len - sizeof(vc_req->status);
+	sg_copy_from_buffer(akcipher_req->dst, sg_nents(akcipher_req->dst),
+			    vc_akcipher_req->dst_buf, akcipher_req->dst_len);
 	virtio_crypto_akcipher_finalize_req(vc_akcipher_req, akcipher_req, error);
 }
 
@@ -230,36 +223,27 @@ static int __virtio_crypto_akcipher_do_req(struct virtio_crypto_akcipher_request
 	int node = dev_to_node(&vcrypto->vdev->dev);
 	unsigned long flags;
 	int ret;
-	bool verify = vc_akcipher_req->opcode == VIRTIO_CRYPTO_AKCIPHER_VERIFY;
-	unsigned int src_len = verify ? req->src_len + req->dst_len : req->src_len;
 
 	/* out header */
 	sg_init_one(&outhdr_sg, req_data, sizeof(*req_data));
 	sgs[num_out++] = &outhdr_sg;
 
 	/* src data */
-	src_buf = kcalloc_node(src_len, 1, GFP_KERNEL, node);
+	src_buf = kcalloc_node(req->src_len, 1, GFP_KERNEL, node);
 	if (!src_buf)
 		return -ENOMEM;
 
-	if (verify) {
-		/* for verify operation, both src and dst data work as OUT direction */
-		sg_copy_to_buffer(req->src, sg_nents(req->src), src_buf, src_len);
-		sg_init_one(&srcdata_sg, src_buf, src_len);
-		sgs[num_out++] = &srcdata_sg;
-	} else {
-		sg_copy_to_buffer(req->src, sg_nents(req->src), src_buf, src_len);
-		sg_init_one(&srcdata_sg, src_buf, src_len);
-		sgs[num_out++] = &srcdata_sg;
+	sg_copy_to_buffer(req->src, sg_nents(req->src), src_buf, req->src_len);
+	sg_init_one(&srcdata_sg, src_buf, req->src_len);
+	sgs[num_out++] = &srcdata_sg;
 
-		/* dst data */
-		dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, node);
-		if (!dst_buf)
-			goto free_src;
+	/* dst data */
+	dst_buf = kcalloc_node(req->dst_len, 1, GFP_KERNEL, node);
+	if (!dst_buf)
+		goto free_src;
 
-		sg_init_one(&dstdata_sg, dst_buf, req->dst_len);
-		sgs[num_out + num_in++] = &dstdata_sg;
-	}
+	sg_init_one(&dstdata_sg, dst_buf, req->dst_len);
+	sgs[num_out + num_in++] = &dstdata_sg;
 
 	vc_akcipher_req->src_buf = src_buf;
 	vc_akcipher_req->dst_buf = dst_buf;
@@ -352,16 +336,6 @@ static int virtio_crypto_rsa_decrypt(struct akcipher_request *req)
 	return virtio_crypto_rsa_req(req, VIRTIO_CRYPTO_AKCIPHER_DECRYPT);
 }
 
-static int virtio_crypto_rsa_sign(struct akcipher_request *req)
-{
-	return virtio_crypto_rsa_req(req, VIRTIO_CRYPTO_AKCIPHER_SIGN);
-}
-
-static int virtio_crypto_rsa_verify(struct akcipher_request *req)
-{
-	return virtio_crypto_rsa_req(req, VIRTIO_CRYPTO_AKCIPHER_VERIFY);
-}
-
 static int virtio_crypto_rsa_set_key(struct crypto_akcipher *tfm,
 				     const void *key,
 				     unsigned int keylen,
@@ -524,16 +498,19 @@ static struct virtio_crypto_akcipher_algo virtio_crypto_akcipher_algs[] = {
 		.algo.base = {
 			.encrypt = virtio_crypto_rsa_encrypt,
 			.decrypt = virtio_crypto_rsa_decrypt,
-			.sign = virtio_crypto_rsa_sign,
-			.verify = virtio_crypto_rsa_verify,
+			/*
+			 * Must specify an arbitrary hash algorithm upon
+			 * set_{pub,priv}_key (even though it's not used
+			 * by encrypt/decrypt) because qemu checks for it.
+			 */
 			.set_pub_key = virtio_crypto_p1pad_rsa_sha1_set_pub_key,
 			.set_priv_key = virtio_crypto_p1pad_rsa_sha1_set_priv_key,
 			.max_size = virtio_crypto_rsa_max_size,
 			.init = virtio_crypto_rsa_init_tfm,
 			.exit = virtio_crypto_rsa_exit_tfm,
 			.base = {
-				.cra_name = "pkcs1pad(rsa,sha1)",
-				.cra_driver_name = "virtio-pkcs1-rsa-with-sha1",
+				.cra_name = "pkcs1pad(rsa)",
+				.cra_driver_name = "virtio-pkcs1-rsa",
 				.cra_priority = 150,
 				.cra_module = THIS_MODULE,
 				.cra_ctxsize = sizeof(struct virtio_crypto_akcipher_ctx),
diff --git a/include/uapi/linux/virtio_crypto.h b/include/uapi/linux/virtio_crypto.h
index 71a54a6849ca..2fccb64c9d6b 100644
--- a/include/uapi/linux/virtio_crypto.h
+++ b/include/uapi/linux/virtio_crypto.h
@@ -329,6 +329,7 @@ struct virtio_crypto_op_header {
 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x00)
 #define VIRTIO_CRYPTO_AKCIPHER_DECRYPT \
 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x01)
+	/* akcipher sign/verify opcodes are deprecated */
 #define VIRTIO_CRYPTO_AKCIPHER_SIGN \
 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x02)
 #define VIRTIO_CRYPTO_AKCIPHER_VERIFY \
-- 
2.43.0

Re: [PATCH v2 00/19] Migrate to sig_alg and templatize ecdsa

[Lukas Wunner]

Hi Herbert,

On Tue, Sep 10, 2024 at 04:30:10PM +0200, Lukas Wunner wrote:
> The original impetus of this series is to introduce P1363 signature
> decoding for ecdsa (patch [18/19]), which is needed by the upcoming
> SPDM library (Security Protocol and Data Model) for PCI device
> authentication.
> 
> To facilitate that, move X9.62 signature decoding out of ecdsa.c and
> into a template (patch [15/19]).
> 
> New in v2:  Move the maximum signature size calculations for ecdsa
> out of software_key_query() and into the X9.62 template so that
> corresponding calculations can be added for P1363 without further
> cluttering up software_key_query() (patch [16/19] - [17/19]).
> 
> New in v2:  Avoid inefficient copying from kernel buffers to sglists
> in the new templates by introducing a sig_alg backend and migrating
> all algorithms to it, per Herbert's advice (patch [02/19] - [12/19]).
> 
> Clean up various smaller issues that caught my eye in ecdsa
> (patch [01/19] and [14/19]), ecrdsa (patch [19/19]) and
> ASN.1 headers (patch [13/19]).

This series was submitted at the tail end of the v6.11 cycle.
It still applies cleanly to v6.12-rc1 though, so I'm not sure
whether to resubmit.

Is there anything you want me to change?

Thanks!

Lukas

Re: [PATCH v2 00/19] Migrate to sig_alg and templatize ecdsa

[Herbert Xu]

On Tue, Sep 10, 2024 at 04:30:10PM +0200, Lukas Wunner wrote:
> The original impetus of this series is to introduce P1363 signature
> decoding for ecdsa (patch [18/19]), which is needed by the upcoming
> SPDM library (Security Protocol and Data Model) for PCI device
> authentication.
> 
> To facilitate that, move X9.62 signature decoding out of ecdsa.c and
> into a template (patch [15/19]).
> 
> New in v2:  Move the maximum signature size calculations for ecdsa
> out of software_key_query() and into the X9.62 template so that
> corresponding calculations can be added for P1363 without further
> cluttering up software_key_query() (patch [16/19] - [17/19]).
> 
> New in v2:  Avoid inefficient copying from kernel buffers to sglists
> in the new templates by introducing a sig_alg backend and migrating
> all algorithms to it, per Herbert's advice (patch [02/19] - [12/19]).
> 
> Clean up various smaller issues that caught my eye in ecdsa
> (patch [01/19] and [14/19]), ecrdsa (patch [19/19]) and
> ASN.1 headers (patch [13/19]).
> 
> I've also accumulated various cleanups for crypto virtio on my
> development branch but will leave them for another day as this
> series is already nearing the "too big to review" threshold. ;)
> 
> I've run selftests on every single commit, but further testing
> would be appreciated to raise the confidence.
> 
> 
> Link to v1:
> 
> https://lore.kernel.org/all/cover.1722260176.git.lukas@wunner.de/
> 
> Changes v1 -> v2:
> 
> * [PATCH 13/19] ASN.1: Clean up include statements in public headers
>   * Drop "#include <linux/bug.h>" from <linux/asn1_encoder.h> (Jonathan)
> 
> * [PATCH 14/19] crypto: ecdsa - Avoid signed integer overflow on signature
>   decoding
>   * Add code comment explaining why vlen may be larger than bufsize (Stefan)
> 
> * [PATCH 15/19] crypto: ecdsa - Move X9.62 signature decoding into template
>   * Drop unnecessary "params", "param_len" and "algo" definitions from
>     ecdsa_nist_p{192,256,384,521}_tv_template[].
>   * Introduce and use struct ecdsa_raw_sig in <crypto/internal/ecc.h>.
> 
> * [PATCH 18/19] crypto: ecdsa - Support P1363 signature decoding
>   * Drop unnecessary "params", "param_len" and "algo" definitions from
>     p1363_ecdsa_nist_p256_tv_template[].
> 
> 
> Lukas Wunner (19):
>   crypto: ecdsa - Drop unused test vector elements
>   crypto: sig - Introduce sig_alg backend
>   crypto: ecdsa - Migrate to sig_alg backend
>   crypto: ecrdsa - Migrate to sig_alg backend
>   crypto: rsa-pkcs1pad - Deduplicate set_{pub,priv}_key callbacks
>   crypto: rsassa-pkcs1 - Migrate to sig_alg backend
>   crypto: rsassa-pkcs1 - Harden digest length verification
>   crypto: rsassa-pkcs1 - Avoid copying hash prefix
>   crypto: virtio - Drop sign/verify operations
>   crypto: drivers - Drop sign/verify operations
>   crypto: akcipher - Drop sign/verify operations
>   crypto: sig - Move crypto_sig_*() API calls to include file
>   ASN.1: Clean up include statements in public headers
>   crypto: ecdsa - Avoid signed integer overflow on signature decoding
>   crypto: ecdsa - Move X9.62 signature decoding into template
>   crypto: sig - Rename crypto_sig_maxsize() to crypto_sig_keysize()
>   crypto: ecdsa - Move X9.62 signature size calculation into template
>   crypto: ecdsa - Support P1363 signature decoding
>   crypto: ecrdsa - Fix signature size calculation
> 
>  Documentation/crypto/api-akcipher.rst         |   2 +-
>  Documentation/crypto/api-sig.rst              |  15 +
>  Documentation/crypto/api.rst                  |   1 +
>  Documentation/crypto/architecture.rst         |   2 +
>  crypto/Kconfig                                |   5 +-
>  crypto/Makefile                               |   5 +-
>  crypto/akcipher.c                             |  64 +-
>  crypto/asymmetric_keys/public_key.c           |  58 +-
>  crypto/ecdsa-p1363.c                          | 159 ++++
>  crypto/ecdsa-x962.c                           | 237 +++++
>  crypto/ecdsa.c                                | 209 ++---
>  crypto/ecrdsa.c                               |  64 +-
>  crypto/internal.h                             |  19 -
>  crypto/rsa-pkcs1pad.c                         | 371 +-------
>  crypto/rsa.c                                  |  17 +-
>  crypto/rsassa-pkcs1.c                         | 442 +++++++++
>  crypto/sig.c                                  | 143 +--
>  crypto/testmgr.c                              | 320 +++++--
>  crypto/testmgr.h                              | 884 +++++++++++++++---
>  drivers/crypto/aspeed/aspeed-acry.c           |   2 -
>  drivers/crypto/hisilicon/hpre/hpre_crypto.c   |   2 -
>  drivers/crypto/starfive/jh7110-rsa.c          |   2 -
>  .../virtio/virtio_crypto_akcipher_algs.c      |  65 +-
>  include/crypto/akcipher.h                     |  69 +-
>  include/crypto/internal/akcipher.h            |   4 +-
>  include/crypto/internal/ecc.h                 |  14 +
>  include/crypto/internal/rsa.h                 |  29 +
>  include/crypto/internal/sig.h                 |  80 ++
>  include/crypto/sig.h                          | 152 ++-
>  include/linux/asn1_decoder.h                  |   1 +
>  include/linux/asn1_encoder.h                  |   1 -
>  include/linux/slab.h                          |   1 +
>  include/uapi/linux/cryptouser.h               |   5 +
>  include/uapi/linux/virtio_crypto.h            |   1 +
>  security/integrity/ima/ima_main.c             |   6 +-
>  35 files changed, 2398 insertions(+), 1053 deletions(-)
>  create mode 100644 Documentation/crypto/api-sig.rst
>  create mode 100644 crypto/ecdsa-p1363.c
>  create mode 100644 crypto/ecdsa-x962.c
>  create mode 100644 crypto/rsassa-pkcs1.c
> 
> -- 
> 2.43.0

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt