[PATCH 0/3] ecdsa KEYCTL_PKEY_QUERY fixes

[PATCH 0/3] ecdsa KEYCTL_PKEY_QUERY fixes

[Lukas Wunner]

For ecdsa, KEYCTL_PKEY_QUERY reports nonsensical values for
enc/dec size and (for P521 keys) also the key size.

Since these are mostly just cosmetic issues, I recommend
not applying for v6.13, but rather let the patches soak
in linux-next.

Thanks!

Lukas Wunner (3):
  crypto: sig - Prepare for algorithms with variable signature size
  crypto: ecdsa - Fix enc/dec size reported by KEYCTL_PKEY_QUERY
  crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

 crypto/asymmetric_keys/public_key.c | 22 +++++++++++-----------
 crypto/ecdsa-p1363.c                |  4 ++--
 crypto/ecdsa-x962.c                 |  4 ++--
 crypto/ecdsa.c                      |  2 +-
 crypto/ecrdsa.c                     |  2 +-
 crypto/rsassa-pkcs1.c               |  4 ++--
 crypto/sig.c                        |  9 +++++++--
 crypto/testmgr.c                    |  7 ++++---
 include/crypto/sig.h                |  7 ++++---
 9 files changed, 34 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH 1/3] crypto: sig - Prepare for algorithms with variable signature size

[Lukas Wunner]

The callers of crypto_sig_sign() assume that the signature size is
always equivalent to the key size.

This happens to be true for RSA, which is currently the only algorithm
implementing the ->sign() callback.  But it is false e.g. for X9.62
encoded ECDSA signatures because they have variable length.

Prepare for addition of a ->sign() callback to such algorithms by
letting the callback return the signature size (or a negative integer
on error).  When testing the ->sign() callback in test_sig_one(),
use crypto_sig_maxsize() instead of crypto_sig_keysize() to verify that
the test vector's signature does not exceed an algorithm's maximum
signature size.

There has been a relatively recent effort to upstream ECDSA signature
generation support which may benefit from this change:

https://lore.kernel.org/linux-crypto/20220908200036.2034-1-ignat@cloudflare.com/

However the main motivation for this commit is to reduce the number of
crypto_sig_keysize() callers:  This function is about to be changed to
return the size in bits instead of bytes and that will require amending
most callers to divide the return value by 8.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Ignat Korchagin <ignat@cloudflare.com>
---
 crypto/asymmetric_keys/public_key.c | 9 ++-------
 crypto/rsassa-pkcs1.c               | 2 +-
 crypto/testmgr.c                    | 7 ++++---
 include/crypto/sig.h                | 5 +++--
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index bbd07a9022e6..bf165d321440 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -267,7 +267,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
 	struct crypto_sig *sig;
 	char *key, *ptr;
 	bool issig;
-	int ksz;
 	int ret;
 
 	pr_devel("==>%s()\n", __func__);
@@ -302,8 +301,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
 			ret = crypto_sig_set_pubkey(sig, key, pkey->keylen);
 		if (ret)
 			goto error_free_tfm;
-
-		ksz = crypto_sig_keysize(sig);
 	} else {
 		tfm = crypto_alloc_akcipher(alg_name, 0, 0);
 		if (IS_ERR(tfm)) {
@@ -317,8 +314,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
 			ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
 		if (ret)
 			goto error_free_tfm;
-
-		ksz = crypto_akcipher_maxsize(tfm);
 	}
 
 	ret = -EINVAL;
@@ -347,8 +342,8 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
 		BUG();
 	}
 
-	if (ret == 0)
-		ret = ksz;
+	if (!issig && ret == 0)
+		ret = crypto_akcipher_maxsize(tfm);
 
 error_free_tfm:
 	if (issig)
diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
index f68ffd338f48..d01ac75635e0 100644
--- a/crypto/rsassa-pkcs1.c
+++ b/crypto/rsassa-pkcs1.c
@@ -210,7 +210,7 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
 		memset(dst, 0, pad_len);
 	}
 
-	return 0;
+	return ctx->key_size;
 }
 
 static int rsassa_pkcs1_verify(struct crypto_sig *tfm,
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 1f5f48ab18c7..76c013bcebe5 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -4339,7 +4339,7 @@ static int test_sig_one(struct crypto_sig *tfm, const struct sig_testvec *vecs)
 	if (vecs->public_key_vec)
 		return 0;
 
-	sig_size = crypto_sig_keysize(tfm);
+	sig_size = crypto_sig_maxsize(tfm);
 	if (sig_size < vecs->c_size) {
 		pr_err("alg: sig: invalid maxsize %u\n", sig_size);
 		return -EINVAL;
@@ -4351,13 +4351,14 @@ static int test_sig_one(struct crypto_sig *tfm, const struct sig_testvec *vecs)
 
 	/* Run asymmetric signature generation */
 	err = crypto_sig_sign(tfm, vecs->m, vecs->m_size, sig, sig_size);
-	if (err) {
+	if (err < 0) {
 		pr_err("alg: sig: sign test failed: err %d\n", err);
 		return err;
 	}
 
 	/* Verify that generated signature equals cooked signature */
-	if (memcmp(sig, vecs->c, vecs->c_size) ||
+	if (err != vecs->c_size ||
+	    memcmp(sig, vecs->c, vecs->c_size) ||
 	    memchr_inv(sig + vecs->c_size, 0, sig_size - vecs->c_size)) {
 		pr_err("alg: sig: sign test failed: invalid output\n");
 		hexdump(sig, sig_size);
diff --git a/include/crypto/sig.h b/include/crypto/sig.h
index cff41ad93824..11024708c069 100644
--- a/include/crypto/sig.h
+++ b/include/crypto/sig.h
@@ -23,7 +23,8 @@ struct crypto_sig {
  * struct sig_alg - generic public key signature algorithm
  *
  * @sign:	Function performs a sign operation as defined by public key
- *		algorithm. Optional.
+ *		algorithm. On success, the signature size is returned.
+ *		Optional.
  * @verify:	Function performs a complete verify operation as defined by
  *		public key algorithm, returning verification status. Optional.
  * @set_pub_key: Function invokes the algorithm specific set public key
@@ -186,7 +187,7 @@ static inline unsigned int crypto_sig_maxsize(struct crypto_sig *tfm)
  * @dst:	destination obuffer
  * @dlen:	destination length
  *
- * Return: zero on success; error code in case of error
+ * Return: signature size on success; error code in case of error
  */
 static inline int crypto_sig_sign(struct crypto_sig *tfm,
 				  const void *src, unsigned int slen,
-- 
2.43.0

Re: [PATCH 1/3] crypto: sig - Prepare for algorithms with variable signature size

[Stefan Berger]

On 12/26/24 1:08 PM, Lukas Wunner wrote:
> The callers of crypto_sig_sign() assume that the signature size is
> always equivalent to the key size.
> 
> This happens to be true for RSA, which is currently the only algorithm
> implementing the ->sign() callback.  But it is false e.g. for X9.62
> encoded ECDSA signatures because they have variable length.
> 
> Prepare for addition of a ->sign() callback to such algorithms by
> letting the callback return the signature size (or a negative integer
> on error).  When testing the ->sign() callback in test_sig_one(),
> use crypto_sig_maxsize() instead of crypto_sig_keysize() to verify that
> the test vector's signature does not exceed an algorithm's maximum
> signature size.
> 
> There has been a relatively recent effort to upstream ECDSA signature
> generation support which may benefit from this change:
> 
> https://lore.kernel.org/linux-crypto/20220908200036.2034-1-ignat@cloudflare.com/
> 
> However the main motivation for this commit is to reduce the number of
> crypto_sig_keysize() callers:  This function is about to be changed to
> return the size in bits instead of bytes and that will require amending
> most callers to divide the return value by 8.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Ignat Korchagin <ignat@cloudflare.com>
> ---
>   crypto/asymmetric_keys/public_key.c | 9 ++-------
>   crypto/rsassa-pkcs1.c               | 2 +-
>   crypto/testmgr.c                    | 7 ++++---
>   include/crypto/sig.h                | 5 +++--
>   4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index bbd07a9022e6..bf165d321440 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -267,7 +267,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>   	struct crypto_sig *sig;
>   	char *key, *ptr;
>   	bool issig;
> -	int ksz;
>   	int ret;
>   
>   	pr_devel("==>%s()\n", __func__);
> @@ -302,8 +301,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>   			ret = crypto_sig_set_pubkey(sig, key, pkey->keylen);
>   		if (ret)
>   			goto error_free_tfm;
> -
> -		ksz = crypto_sig_keysize(sig);
>   	} else {
>   		tfm = crypto_alloc_akcipher(alg_name, 0, 0);
>   		if (IS_ERR(tfm)) {
> @@ -317,8 +314,6 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>   			ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
>   		if (ret)
>   			goto error_free_tfm;
> -
> -		ksz = crypto_akcipher_maxsize(tfm);
>   	}
>   
>   	ret = -EINVAL;
> @@ -347,8 +342,8 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>   		BUG();
>   	}
>   
> -	if (ret == 0)
> -		ret = ksz;
> +	if (!issig && ret == 0)
> +		ret = crypto_akcipher_maxsize(tfm);
>   
>   error_free_tfm:
>   	if (issig)
> diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
> index f68ffd338f48..d01ac75635e0 100644
> --- a/crypto/rsassa-pkcs1.c
> +++ b/crypto/rsassa-pkcs1.c
> @@ -210,7 +210,7 @@ static int rsassa_pkcs1_sign(struct crypto_sig *tfm,
>   		memset(dst, 0, pad_len);
>   	}
>   
> -	return 0;
> +	return ctx->key_size;
>   }
>   
>   static int rsassa_pkcs1_verify(struct crypto_sig *tfm,
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 1f5f48ab18c7..76c013bcebe5 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -4339,7 +4339,7 @@ static int test_sig_one(struct crypto_sig *tfm, const struct sig_testvec *vecs)
>   	if (vecs->public_key_vec)
>   		return 0;
>   
> -	sig_size = crypto_sig_keysize(tfm);
> +	sig_size = crypto_sig_maxsize(tfm);
>   	if (sig_size < vecs->c_size) {
>   		pr_err("alg: sig: invalid maxsize %u\n", sig_size);
>   		return -EINVAL;
> @@ -4351,13 +4351,14 @@ static int test_sig_one(struct crypto_sig *tfm, const struct sig_testvec *vecs)
>   
>   	/* Run asymmetric signature generation */
>   	err = crypto_sig_sign(tfm, vecs->m, vecs->m_size, sig, sig_size);
> -	if (err) {
> +	if (err < 0) {
>   		pr_err("alg: sig: sign test failed: err %d\n", err);
>   		return err;
>   	}
>   
>   	/* Verify that generated signature equals cooked signature */
> -	if (memcmp(sig, vecs->c, vecs->c_size) ||
> +	if (err != vecs->c_size ||
> +	    memcmp(sig, vecs->c, vecs->c_size) ||
>   	    memchr_inv(sig + vecs->c_size, 0, sig_size - vecs->c_size)) {
>   		pr_err("alg: sig: sign test failed: invalid output\n");
>   		hexdump(sig, sig_size);
> diff --git a/include/crypto/sig.h b/include/crypto/sig.h
> index cff41ad93824..11024708c069 100644
> --- a/include/crypto/sig.h
> +++ b/include/crypto/sig.h
> @@ -23,7 +23,8 @@ struct crypto_sig {
>    * struct sig_alg - generic public key signature algorithm
>    *
>    * @sign:	Function performs a sign operation as defined by public key
> - *		algorithm. Optional.
> + *		algorithm. On success, the signature size is returned.
> + *		Optional.
>    * @verify:	Function performs a complete verify operation as defined by
>    *		public key algorithm, returning verification status. Optional.
>    * @set_pub_key: Function invokes the algorithm specific set public key
> @@ -186,7 +187,7 @@ static inline unsigned int crypto_sig_maxsize(struct crypto_sig *tfm)
>    * @dst:	destination obuffer
>    * @dlen:	destination length
>    *
> - * Return: zero on success; error code in case of error
> + * Return: signature size on success; error code in case of error
>    */
>   static inline int crypto_sig_sign(struct crypto_sig *tfm,
>   				  const void *src, unsigned int slen,

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

[PATCH 2/3] crypto: ecdsa - Fix enc/dec size reported by KEYCTL_PKEY_QUERY

[Lukas Wunner]

KEYCTL_PKEY_QUERY system calls for ecdsa keys return the key size as
max_enc_size and max_dec_size, even though such keys cannot be used for
encryption/decryption.  They're exclusively for signature generation or
verification.

Only rsa keys with pkcs1 encoding can also be used for encryption or
decryption.

Return 0 instead for ecdsa keys (as well as ecrdsa keys).

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 crypto/asymmetric_keys/public_key.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index bf165d321440..dd44a966947f 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -188,6 +188,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
 	ptr = pkey_pack_u32(ptr, pkey->paramlen);
 	memcpy(ptr, pkey->params, pkey->paramlen);
 
+	memset(info, 0, sizeof(*info));
+
 	if (issig) {
 		sig = crypto_alloc_sig(alg_name, 0, 0);
 		if (IS_ERR(sig)) {
@@ -211,6 +213,9 @@ static int software_key_query(const struct kernel_pkey_params *params,
 			info->supported_ops |= KEYCTL_SUPPORTS_SIGN;
 
 		if (strcmp(params->encoding, "pkcs1") == 0) {
+			info->max_enc_size = len;
+			info->max_dec_size = len;
+
 			info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT;
 			if (pkey->key_is_private)
 				info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT;
@@ -232,6 +237,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
 		len = crypto_akcipher_maxsize(tfm);
 		info->max_sig_size = len;
 		info->max_data_size = len;
+		info->max_enc_size = len;
+		info->max_dec_size = len;
 
 		info->supported_ops = KEYCTL_SUPPORTS_ENCRYPT;
 		if (pkey->key_is_private)
@@ -239,8 +246,6 @@ static int software_key_query(const struct kernel_pkey_params *params,
 	}
 
 	info->key_size = len * 8;
-	info->max_enc_size = len;
-	info->max_dec_size = len;
 
 	ret = 0;
 
-- 
2.43.0

Re: [PATCH 2/3] crypto: ecdsa - Fix enc/dec size reported by KEYCTL_PKEY_QUERY

[Stefan Berger]

On 12/26/24 1:08 PM, Lukas Wunner wrote:
> KEYCTL_PKEY_QUERY system calls for ecdsa keys return the key size as
> max_enc_size and max_dec_size, even though such keys cannot be used for
> encryption/decryption.  They're exclusively for signature generation or
> verification.
> 
> Only rsa keys with pkcs1 encoding can also be used for encryption or
> decryption.
> 
> Return 0 instead for ecdsa keys (as well as ecrdsa keys).
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   crypto/asymmetric_keys/public_key.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index bf165d321440..dd44a966947f 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -188,6 +188,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   	ptr = pkey_pack_u32(ptr, pkey->paramlen);
>   	memcpy(ptr, pkey->params, pkey->paramlen);
>   
> +	memset(info, 0, sizeof(*info));
> +
>   	if (issig) {
>   		sig = crypto_alloc_sig(alg_name, 0, 0);
>   		if (IS_ERR(sig)) {
> @@ -211,6 +213,9 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			info->supported_ops |= KEYCTL_SUPPORTS_SIGN;
>   
>   		if (strcmp(params->encoding, "pkcs1") == 0) {
> +			info->max_enc_size = len;
> +			info->max_dec_size = len;
> +
>   			info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT;
>   			if (pkey->key_is_private)
>   				info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT;
> @@ -232,6 +237,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   		len = crypto_akcipher_maxsize(tfm);
>   		info->max_sig_size = len;
>   		info->max_data_size = len;
> +		info->max_enc_size = len;
> +		info->max_dec_size = len;
>   
>   		info->supported_ops = KEYCTL_SUPPORTS_ENCRYPT;
>   		if (pkey->key_is_private)
> @@ -239,8 +246,6 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   	}
>   
>   	info->key_size = len * 8;
> -	info->max_enc_size = len;
> -	info->max_dec_size = len;
>   
>   	ret = 0;
>   

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

[PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Lukas Wunner]

When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
key, the key_size is incorrectly reported as 528 bits instead of 521.

That's because the key size obtained through crypto_sig_keysize() is in
bytes and software_key_query() multiplies by 8 to yield the size in bits.
The underlying assumption is that the key size is always a multiple of 8.
With the recent addition of NIST P521, that's no longer the case.

Fix by returning the key_size in bits from crypto_sig_keysize() and
adjusting the calculations in software_key_query().

The ->key_size() callbacks of sig_alg algorithms now return the size in
bits, whereas the ->digest_size() and ->max_size() callbacks return the
size in bytes.  This matches with the units in struct keyctl_pkey_query.

Fixes: a7d45ba77d3d ("crypto: ecdsa - Register NIST P521 and extend test suite")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 crypto/asymmetric_keys/public_key.c | 8 ++++----
 crypto/ecdsa-p1363.c                | 4 ++--
 crypto/ecdsa-x962.c                 | 4 ++--
 crypto/ecdsa.c                      | 2 +-
 crypto/ecrdsa.c                     | 2 +-
 crypto/rsassa-pkcs1.c               | 2 +-
 crypto/sig.c                        | 9 +++++++--
 include/crypto/sig.h                | 2 +-
 8 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index dd44a966947f..6cafb2b8aa22 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -205,6 +205,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
 			goto error_free_tfm;
 
 		len = crypto_sig_keysize(sig);
+		info->key_size = len;
 		info->max_sig_size = crypto_sig_maxsize(sig);
 		info->max_data_size = crypto_sig_digestsize(sig);
 
@@ -213,8 +214,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
 			info->supported_ops |= KEYCTL_SUPPORTS_SIGN;
 
 		if (strcmp(params->encoding, "pkcs1") == 0) {
-			info->max_enc_size = len;
-			info->max_dec_size = len;
+			info->max_enc_size = len / 8;
+			info->max_dec_size = len / 8;
 
 			info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT;
 			if (pkey->key_is_private)
@@ -235,6 +236,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
 			goto error_free_tfm;
 
 		len = crypto_akcipher_maxsize(tfm);
+		info->key_size = len * 8;
 		info->max_sig_size = len;
 		info->max_data_size = len;
 		info->max_enc_size = len;
@@ -245,8 +247,6 @@ static int software_key_query(const struct kernel_pkey_params *params,
 			info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT;
 	}
 
-	info->key_size = len * 8;
-
 	ret = 0;
 
 error_free_tfm:
diff --git a/crypto/ecdsa-p1363.c b/crypto/ecdsa-p1363.c
index eaae7214d69b..c4f458df18ed 100644
--- a/crypto/ecdsa-p1363.c
+++ b/crypto/ecdsa-p1363.c
@@ -21,7 +21,7 @@ static int ecdsa_p1363_verify(struct crypto_sig *tfm,
 			      const void *digest, unsigned int dlen)
 {
 	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
-	unsigned int keylen = crypto_sig_keysize(ctx->child);
+	unsigned int keylen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
 	unsigned int ndigits = DIV_ROUND_UP(keylen, sizeof(u64));
 	struct ecdsa_raw_sig sig;
 
@@ -45,7 +45,7 @@ static unsigned int ecdsa_p1363_max_size(struct crypto_sig *tfm)
 {
 	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
 
-	return 2 * crypto_sig_keysize(ctx->child);
+	return 2 * DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
 }
 
 static unsigned int ecdsa_p1363_digest_size(struct crypto_sig *tfm)
diff --git a/crypto/ecdsa-x962.c b/crypto/ecdsa-x962.c
index 6a77c13e192b..0327e1441374 100644
--- a/crypto/ecdsa-x962.c
+++ b/crypto/ecdsa-x962.c
@@ -82,7 +82,7 @@ static int ecdsa_x962_verify(struct crypto_sig *tfm,
 	int err;
 
 	sig_ctx.ndigits = DIV_ROUND_UP(crypto_sig_keysize(ctx->child),
-				       sizeof(u64));
+				       sizeof(u64) * 8);
 
 	err = asn1_ber_decoder(&ecdsasignature_decoder, &sig_ctx, src, slen);
 	if (err < 0)
@@ -103,7 +103,7 @@ static unsigned int ecdsa_x962_max_size(struct crypto_sig *tfm)
 {
 	struct ecdsa_x962_ctx *ctx = crypto_sig_ctx(tfm);
 	struct sig_alg *alg = crypto_sig_alg(ctx->child);
-	int slen = crypto_sig_keysize(ctx->child);
+	int slen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
 
 	/*
 	 * Verify takes ECDSA-Sig-Value (described in RFC 5480) as input,
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
index 117526d15dde..a70b60a90a3c 100644
--- a/crypto/ecdsa.c
+++ b/crypto/ecdsa.c
@@ -167,7 +167,7 @@ static unsigned int ecdsa_key_size(struct crypto_sig *tfm)
 {
 	struct ecc_ctx *ctx = crypto_sig_ctx(tfm);
 
-	return DIV_ROUND_UP(ctx->curve->nbits, 8);
+	return ctx->curve->nbits;
 }
 
 static unsigned int ecdsa_digest_size(struct crypto_sig *tfm)
diff --git a/crypto/ecrdsa.c b/crypto/ecrdsa.c
index b3dd8a3ddeb7..53c9fd9f807f 100644
--- a/crypto/ecrdsa.c
+++ b/crypto/ecrdsa.c
@@ -249,7 +249,7 @@ static unsigned int ecrdsa_key_size(struct crypto_sig *tfm)
 	 * Verify doesn't need any output, so it's just informational
 	 * for keyctl to determine the key bit size.
 	 */
-	return ctx->pub_key.ndigits * sizeof(u64);
+	return ctx->pub_key.ndigits * sizeof(u64) * 8;
 }
 
 static unsigned int ecrdsa_max_size(struct crypto_sig *tfm)
diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
index d01ac75635e0..299b2512cc95 100644
--- a/crypto/rsassa-pkcs1.c
+++ b/crypto/rsassa-pkcs1.c
@@ -301,7 +301,7 @@ static unsigned int rsassa_pkcs1_key_size(struct crypto_sig *tfm)
 {
 	struct rsassa_pkcs1_ctx *ctx = crypto_sig_ctx(tfm);
 
-	return ctx->key_size;
+	return ctx->key_size * 8;
 }
 
 static int rsassa_pkcs1_set_pub_key(struct crypto_sig *tfm,
diff --git a/crypto/sig.c b/crypto/sig.c
index dfc7cae90802..7399e67c6f12 100644
--- a/crypto/sig.c
+++ b/crypto/sig.c
@@ -102,6 +102,11 @@ static int sig_default_set_key(struct crypto_sig *tfm,
 	return -ENOSYS;
 }
 
+static unsigned int sig_default_size(struct crypto_sig *tfm)
+{
+	return DIV_ROUND_UP(crypto_sig_keysize(tfm), 8);
+}
+
 static int sig_prepare_alg(struct sig_alg *alg)
 {
 	struct crypto_alg *base = &alg->base;
@@ -117,9 +122,9 @@ static int sig_prepare_alg(struct sig_alg *alg)
 	if (!alg->key_size)
 		return -EINVAL;
 	if (!alg->max_size)
-		alg->max_size = alg->key_size;
+		alg->max_size = sig_default_size;
 	if (!alg->digest_size)
-		alg->digest_size = alg->key_size;
+		alg->digest_size = sig_default_size;
 
 	base->cra_type = &crypto_sig_type;
 	base->cra_flags &= ~CRYPTO_ALG_TYPE_MASK;
diff --git a/include/crypto/sig.h b/include/crypto/sig.h
index 11024708c069..fa6dafafab3f 100644
--- a/include/crypto/sig.h
+++ b/include/crypto/sig.h
@@ -128,7 +128,7 @@ static inline void crypto_free_sig(struct crypto_sig *tfm)
 /**
  * crypto_sig_keysize() - Get key size
  *
- * Function returns the key size in bytes.
+ * Function returns the key size in bits.
  * Function assumes that the key is already set in the transformation. If this
  * function is called without a setkey or with a failed setkey, you may end up
  * in a NULL dereference.
-- 
2.43.0

Re: [PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Stefan Berger]

On 12/26/24 1:08 PM, Lukas Wunner wrote:
> When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
> key, the key_size is incorrectly reported as 528 bits instead of 521.

Is there a way to query this with keyctl pkey_query?

 > > That's because the key size obtained through crypto_sig_keysize() is in
> bytes and software_key_query() multiplies by 8 to yield the size in bits.
> The underlying assumption is that the key size is always a multiple of 8.
> With the recent addition of NIST P521, that's no longer the case.
> 
> Fix by returning the key_size in bits from crypto_sig_keysize() and
> adjusting the calculations in software_key_query().
> 
> The ->key_size() callbacks of sig_alg algorithms now return the size in
> bits, whereas the ->digest_size() and ->max_size() callbacks return the
> size in bytes.  This matches with the units in struct keyctl_pkey_query.
> 
> Fixes: a7d45ba77d3d ("crypto: ecdsa - Register NIST P521 and extend test suite")
 > Signed-off-by: Lukas Wunner <lukas@wunner.de>> ---
>   crypto/asymmetric_keys/public_key.c | 8 ++++----
>   crypto/ecdsa-p1363.c                | 4 ++--
>   crypto/ecdsa-x962.c                 | 4 ++--
>   crypto/ecdsa.c                      | 2 +-
>   crypto/ecrdsa.c                     | 2 +-
>   crypto/rsassa-pkcs1.c               | 2 +-
>   crypto/sig.c                        | 9 +++++++--
>   include/crypto/sig.h                | 2 +-
>   8 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index dd44a966947f..6cafb2b8aa22 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -205,6 +205,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			goto error_free_tfm;
>   
>   		len = crypto_sig_keysize(sig);
> +		info->key_size = len;
>   		info->max_sig_size = crypto_sig_maxsize(sig);
>   		info->max_data_size = crypto_sig_digestsize(sig);
>   
> @@ -213,8 +214,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			info->supported_ops |= KEYCTL_SUPPORTS_SIGN;
>   
>   		if (strcmp(params->encoding, "pkcs1") == 0) {
> -			info->max_enc_size = len;
> -			info->max_dec_size = len;
> +			info->max_enc_size = len / 8;
> +			info->max_dec_size = len / 8;
>   
>   			info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT;
>   			if (pkey->key_is_private)
> @@ -235,6 +236,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			goto error_free_tfm;
>   
>   		len = crypto_akcipher_maxsize(tfm);
> +		info->key_size = len * 8;
>   		info->max_sig_size = len;
>   		info->max_data_size = len;
>   		info->max_enc_size = len;
> @@ -245,8 +247,6 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT;
>   	}
>   
> -	info->key_size = len * 8;
> -
>   	ret = 0;
>   
>   error_free_tfm:
> diff --git a/crypto/ecdsa-p1363.c b/crypto/ecdsa-p1363.c
> index eaae7214d69b..c4f458df18ed 100644
> --- a/crypto/ecdsa-p1363.c
> +++ b/crypto/ecdsa-p1363.c
> @@ -21,7 +21,7 @@ static int ecdsa_p1363_verify(struct crypto_sig *tfm,
>   			      const void *digest, unsigned int dlen)
>   {
>   	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
> -	unsigned int keylen = crypto_sig_keysize(ctx->child);
> +	unsigned int keylen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   	unsigned int ndigits = DIV_ROUND_UP(keylen, sizeof(u64));
>   	struct ecdsa_raw_sig sig;
>   
> @@ -45,7 +45,7 @@ static unsigned int ecdsa_p1363_max_size(struct crypto_sig *tfm)
>   {
>   	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return 2 * crypto_sig_keysize(ctx->child);
> +	return 2 * DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   }
>   
>   static unsigned int ecdsa_p1363_digest_size(struct crypto_sig *tfm)
> diff --git a/crypto/ecdsa-x962.c b/crypto/ecdsa-x962.c
> index 6a77c13e192b..0327e1441374 100644
> --- a/crypto/ecdsa-x962.c
> +++ b/crypto/ecdsa-x962.c
> @@ -82,7 +82,7 @@ static int ecdsa_x962_verify(struct crypto_sig *tfm,
>   	int err;
>   
>   	sig_ctx.ndigits = DIV_ROUND_UP(crypto_sig_keysize(ctx->child),
> -				       sizeof(u64));
> +				       sizeof(u64) * 8);
>   
>   	err = asn1_ber_decoder(&ecdsasignature_decoder, &sig_ctx, src, slen);
>   	if (err < 0)
> @@ -103,7 +103,7 @@ static unsigned int ecdsa_x962_max_size(struct crypto_sig *tfm)
>   {
>   	struct ecdsa_x962_ctx *ctx = crypto_sig_ctx(tfm);
>   	struct sig_alg *alg = crypto_sig_alg(ctx->child);
> -	int slen = crypto_sig_keysize(ctx->child);
> +	int slen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   
>   	/*
>   	 * Verify takes ECDSA-Sig-Value (described in RFC 5480) as input,
> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> index 117526d15dde..a70b60a90a3c 100644
> --- a/crypto/ecdsa.c
> +++ b/crypto/ecdsa.c
> @@ -167,7 +167,7 @@ static unsigned int ecdsa_key_size(struct crypto_sig *tfm)
>   {
>   	struct ecc_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return DIV_ROUND_UP(ctx->curve->nbits, 8);
> +	return ctx->curve->nbits;
>   }
>   
>   static unsigned int ecdsa_digest_size(struct crypto_sig *tfm)
> diff --git a/crypto/ecrdsa.c b/crypto/ecrdsa.c
> index b3dd8a3ddeb7..53c9fd9f807f 100644
> --- a/crypto/ecrdsa.c
> +++ b/crypto/ecrdsa.c
> @@ -249,7 +249,7 @@ static unsigned int ecrdsa_key_size(struct crypto_sig *tfm)
>   	 * Verify doesn't need any output, so it's just informational
>   	 * for keyctl to determine the key bit size.
>   	 */
> -	return ctx->pub_key.ndigits * sizeof(u64);
> +	return ctx->pub_key.ndigits * sizeof(u64) * 8;
>   }
>   
>   static unsigned int ecrdsa_max_size(struct crypto_sig *tfm)
> diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
> index d01ac75635e0..299b2512cc95 100644
> --- a/crypto/rsassa-pkcs1.c
> +++ b/crypto/rsassa-pkcs1.c
> @@ -301,7 +301,7 @@ static unsigned int rsassa_pkcs1_key_size(struct crypto_sig *tfm)
>   {
>   	struct rsassa_pkcs1_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return ctx->key_size;
> +	return ctx->key_size * 8;
>   }
>   
>   static int rsassa_pkcs1_set_pub_key(struct crypto_sig *tfm,
> diff --git a/crypto/sig.c b/crypto/sig.c
> index dfc7cae90802..7399e67c6f12 100644
> --- a/crypto/sig.c
> +++ b/crypto/sig.c
> @@ -102,6 +102,11 @@ static int sig_default_set_key(struct crypto_sig *tfm,
>   	return -ENOSYS;
>   }
>   
> +static unsigned int sig_default_size(struct crypto_sig *tfm)
> +{
> +	return DIV_ROUND_UP(crypto_sig_keysize(tfm), 8);
> +}
> +
>   static int sig_prepare_alg(struct sig_alg *alg)
>   {
>   	struct crypto_alg *base = &alg->base;
> @@ -117,9 +122,9 @@ static int sig_prepare_alg(struct sig_alg *alg)
>   	if (!alg->key_size)
>   		return -EINVAL;
>   	if (!alg->max_size)
> -		alg->max_size = alg->key_size;
> +		alg->max_size = sig_default_size;
>   	if (!alg->digest_size)
> -		alg->digest_size = alg->key_size;
> +		alg->digest_size = sig_default_size;
>   
>   	base->cra_type = &crypto_sig_type;
>   	base->cra_flags &= ~CRYPTO_ALG_TYPE_MASK;
> diff --git a/include/crypto/sig.h b/include/crypto/sig.h
> index 11024708c069..fa6dafafab3f 100644
> --- a/include/crypto/sig.h
> +++ b/include/crypto/sig.h
> @@ -128,7 +128,7 @@ static inline void crypto_free_sig(struct crypto_sig *tfm)
>   /**
>    * crypto_sig_keysize() - Get key size
>    *
> - * Function returns the key size in bytes.
> + * Function returns the key size in bits.
>    * Function assumes that the key is already set in the transformation. If this
>    * function is called without a setkey or with a failed setkey, you may end up
>    * in a NULL dereference.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Lukas Wunner]

On Thu, Jan 02, 2025 at 12:45:47PM -0500, Stefan Berger wrote:
> On 12/26/24 1:08 PM, Lukas Wunner wrote:
> > When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
> > key, the key_size is incorrectly reported as 528 bits instead of 521.
> 
> Is there a way to query this with keyctl pkey_query?

Yes, these are the commands I've used for testing:

  id=`keyctl padd asymmetric "" %:_uid.0 < end_responder.cert.der`
  keyctl pkey_query $id 0 enc=x962 hash=sha256

This is the certificate I've used:

  https://github.com/DMTF/libspdm/raw/refs/heads/main/unit_test/sample_key/ecp521/end_responder.cert.der

Before:

  key_size=528
  max_data_size=64
  max_sig_size=139
  max_enc_size=66
  max_dec_size=66
  encrypt=n
  decrypt=n
  sign=n
  verify=y

After:

  key_size=521
  max_data_size=64
  max_sig_size=139
  max_enc_size=0
  max_dec_size=0
  encrypt=n
  decrypt=n
  sign=n
  verify=y

Thanks,

Lukas

Re: [PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Stefan Berger]

On 1/3/25 12:38 PM, Lukas Wunner wrote:
> On Thu, Jan 02, 2025 at 12:45:47PM -0500, Stefan Berger wrote:
>> On 12/26/24 1:08 PM, Lukas Wunner wrote:
>>> When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
>>> key, the key_size is incorrectly reported as 528 bits instead of 521.
>>
>> Is there a way to query this with keyctl pkey_query?
> 
> Yes, these are the commands I've used for testing:
> 
>    id=`keyctl padd asymmetric "" %:_uid.0 < end_responder.cert.der`
>    keyctl pkey_query $id 0 enc=x962 hash=sha256
> 

I had tried with these here as root:

# keyctl show %keyring:.ima
Keyring
  461728044 ---lswrv      0     0  keyring: .ima
  579203092 ---lswrv      0     0   \_ asymmetric: Fedora kernel signing 
key: 50e9f2a484a5b9e7279e7bf7f3ad54b0572c2f1e
  774765589 --als--v      0     0   \_ asymmetric: my rsa signing key: 
69f518ae20dbb4a412f33b8950b2fd1e2b850fd1
   15381609 --als--v      0     0   \_ asymmetric: my ecc signing key: 
0ab4280f3df700f2cb6711b930748e1224eae40d
   72176491 --als--v      0     0   \_ asymmetric: Fedora 42 IMA 
Code-signing cert: a1a5c4c8d90554e0ce5c07c9e127f20362f02aa4
  612838334 --als--v      0     0   \_ asymmetric: Fedora 41 IMA 
Code-signing cert: 158befb98fc2ee070833d1a2a46669e7876d7435
   51623090 --als--v      0     0   \_ asymmetric: Fedora 40 IMA 
Code-signing cert: 2defa2e1d528db308d3e1ca28274aa40a3204a9e
   85986135 --als--v      0     0   \_ asymmetric: Fedora 39 IMA 
Code-signing cert: 155266a4a3ea7bdddc9e38ddb192c2d2388b603e
# keyctl pkey_query 612838334 0 enc=x962
keyctl_pkey_query: Permission denied
# keyctl pkey_query 612838334 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied
# keyctl pkey_query 579203092 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied
# keyctl pkey_query 774765589 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied


> This is the certificate I've used:
> 
>    https://github.com/DMTF/libspdm/raw/refs/heads/main/unit_test/sample_key/ecp521/end_responder.cert.der

# keyctl show
Session Keyring
  377868180 --alswrv      0     0  keyring: _ses
1014059943 --alswrv      0 65534   \_ keyring: _uid.0
  138203159 --als--v      0     0       \_ asymmetric: DMTF libspdm 
ECP521 responder cert: e4bcd74895d3a7bd230ad2a46941c3be6d5c91cc

# keyctl pkey_query $id 0 enc=x962 hash=sha256
key_size=528
max_data_size=64
max_sig_size=139
max_enc_size=66
max_dec_size=66
encrypt=n
decrypt=n
sign=n
verify=y

more favorable permissions - obviously

Thanks!

   Stefan

 > > Before:
> 
>    key_size=528
>    max_data_size=64
>    max_sig_size=139
>    max_enc_size=66
>    max_dec_size=66
>    encrypt=n
>    decrypt=n
>    sign=n
>    verify=y
> 
> After:
> 
>    key_size=521
>    max_data_size=64
>    max_sig_size=139
>    max_enc_size=0
>    max_dec_size=0
>    encrypt=n
>    decrypt=n
>    sign=n
>    verify=y
> 
> Thanks,
> 
> Lukas
> 

Re: [PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Herbert Xu]

On Thu, Dec 26, 2024 at 07:08:03PM +0100, Lukas Wunner wrote:
>
> diff --git a/crypto/ecdsa-p1363.c b/crypto/ecdsa-p1363.c
> index eaae7214d69b..c4f458df18ed 100644
> --- a/crypto/ecdsa-p1363.c
> +++ b/crypto/ecdsa-p1363.c
> @@ -21,7 +21,7 @@ static int ecdsa_p1363_verify(struct crypto_sig *tfm,
>  			      const void *digest, unsigned int dlen)
>  {
>  	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
> -	unsigned int keylen = crypto_sig_keysize(ctx->child);
> +	unsigned int keylen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);

This may overflow unnecessarily, please rewrite these as:

	X / 8 + !!(X & 7)

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

Re: [PATCH 3/3] crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

[Lukas Wunner]

On Sat, Jan 04, 2025 at 08:45:10AM +0800, Herbert Xu wrote:
> On Thu, Dec 26, 2024 at 07:08:03PM +0100, Lukas Wunner wrote:
> >
> > diff --git a/crypto/ecdsa-p1363.c b/crypto/ecdsa-p1363.c
> > index eaae7214d69b..c4f458df18ed 100644
> > --- a/crypto/ecdsa-p1363.c
> > +++ b/crypto/ecdsa-p1363.c
> > @@ -21,7 +21,7 @@ static int ecdsa_p1363_verify(struct crypto_sig *tfm,
> >  			      const void *digest, unsigned int dlen)
> >  {
> >  	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
> > -	unsigned int keylen = crypto_sig_keysize(ctx->child);
> > +	unsigned int keylen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
> 
> This may overflow unnecessarily, please rewrite these as:
> 
> 	X / 8 + !!(X & 7)

Interesting.  Wouldn't it make sense to have a DIV_ROUND_UP_SAFE() macro
for cases like this?

I'd expect this version to actually be faster than DIV_ROUND_UP():
There's the extra logical AND, as well as the negation.
But the "n / d" and "!!(n & (d - 1))" can be computed in parallel.

Thanks,

Lukas