Re: [PATCH v2 2/2] integrity: Add support for sigv3 verification using ML-DSA keys

On 4/14/26 10:01 PM, Mimi Zohar wrote:
> On Wed, 2026-04-08 at 13:41 -0400, Stefan Berger wrote:
>> Add support for sigv3 signature verification using ML-DSA in pure mode.
>> When a sigv3 signature is verified, first check whether the key to use
>> for verification is an ML-DSA key and therefore uses a hashless signature
>> verification scheme. The hashless signature verification method uses the
>> ima_file_id structure directly for signature verification rather than
>> its digest.
>>
>> Suggested-by: Eric Biggers <[email protected]>
>> Signed-off-by: Stefan Berger <[email protected]>
>>
> 
> Thanks, Stefan.
>> ---
>> v2: Set hash_algo in public_key_signature to "none"
>> ---
>>   security/integrity/digsig_asymmetric.c | 84 ++++++++++++++++++++++++--
>>   1 file changed, 79 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
>> index e29ed73f15cd..c80cb2b117a6 100644
>> --- a/security/integrity/digsig_asymmetric.c
>> +++ b/security/integrity/digsig_asymmetric.c
>> @@ -190,17 +190,91 @@ static int calc_file_id_hash(enum evm_ima_xattr_type type,
>>   	return rc;
>>   }
>>   
>> +/*
> 
> kernel-doc starts with "/**".

I followed the pattern of documentation of a static function that you 
just moved:

/*
  * calc_file_id_hash - calculate the hash of the ima_file_id struct data
  * @type: xattr type [enum evm_ima_xattr_type]


> 
>> + * asymmetric_verify_v3_hashless - Use hashless signature verification on sigv3
>> + * @key: The key to use for signature verification
>> + * @pk: The associated public key
>> + * @encoding: The encoding the key type uses
>> + * @sig: The signature
>> + * @siglen: The length of the xattr signature
>> + * @algo: The hash algorithm
>> + * @digest: The file digest
>> + *
>> + * Create an ima_file_id structure and use it for signature verification
>> + * directly. This can be used for ML-DSA in pure mode for example.
> 
> Like the comments on 1/2, please add a comment here indicating that all callers
> must verify the signature length (siglen) and the public key (pk) is not NULL,
> before calling asymmetric_verify_v3_hashless().  Also indicate that the caller
> must free the key.
> 
>> + */
>> +static int asymmetric_verify_v3_hashless(struct key *key,
>> +					 const struct public_key *pk,
>> +					 const char *encoding,
>> +					 const char *sig, int siglen,
>> +					 u8 algo,
>> +					 const u8 *digest)
>> +{
>> +	struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
>> +	struct ima_file_id file_id = {
>> +		.hash_type = hdr->type,
>> +		.hash_algorithm = algo,
>> +	};
>> +	size_t digest_size = hash_digest_size[algo];
> 
> Defer initializing the digest_size and .m_size, below, until after checking the
> hash algorithm is valid.

This function is called by asymmetric_verify. asymmetric_verify calls 
calc_file_id_hash, which doesn't check algo for valid range, either. I 
suppose it's an untrusted value at this point (IMA never checked it's 
value for valid range?) an we should check it in asymmetric_verify then 
to cover both cases? Or you want to check it individually?

> 
>> +	struct public_key_signature pks = {
>> +		.m = (u8 *)&file_id,
>> +		.m_size = sizeof(file_id) - (HASH_MAX_DIGESTSIZE - digest_size),
>> +		.s = hdr->sig,
>> +		.s_size = siglen - sizeof(*hdr),
>> +		.pkey_algo = pk->pkey_algo,
>> +		.hash_algo = "none",
>> +		.encoding = encoding,
>> +	};
>> +	int ret;
>> +
>> +	if (hdr->type != IMA_VERITY_DIGSIG &&
>> +	    hdr->type != EVM_IMA_XATTR_DIGSIG &&
>> +	    hdr->type != EVM_XATTR_PORTABLE_DIGSIG)
>> +		return -EINVAL;
>> +
>> +	if (pks.s_size != be16_to_cpu(hdr->sig_size))
>> +		return -EBADMSG;
>> +
>> +	memcpy(file_id.hash, digest, digest_size);
> 
> First check the hash algorithm is valid, before using digest_size.
> 
>> +
>> +	ret = verify_signature(key, &pks);
>> +	pr_debug("%s() = %d\n", __func__, ret);
>> +	return ret;
>> +}
>> +
>>   int asymmetric_verify_v3(struct key *keyring, const char *sig, int siglen,
>>   			 const char *data, int datalen, u8 algo)
>>   {
>>   	struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
>>   	struct ima_max_digest_data hash;
>> +	const struct public_key *pk;
>> +	struct key *key;
>>   	int rc;
>>   
>> -	rc = calc_file_id_hash(hdr->type, algo, data, &hash);
>> -	if (rc)
>> -		return -EINVAL;
>> +	if (siglen <= sizeof(*hdr))
>> +		return -EBADMSG;
>> +
>> +	key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
>> +	if (IS_ERR(key))
>> +		return PTR_ERR(key);
>>   
>> -	return asymmetric_verify(keyring, sig, siglen, hash.digest,
>> -				 hash.hdr.length);
>> +	pk = asymmetric_key_public_key(key);
> 
> Please add a test to check that 'pk' isn't null.
> 
>> +	if (!strncmp(pk->pkey_algo, "mldsa", 5)) {
>> +		rc = asymmetric_verify_v3_hashless(key, pk, "raw",
>> +						   sig, siglen, algo, data);
>> +	} else {
>> +		rc = calc_file_id_hash(hdr->type, algo, data, &hash);
>> +		if (rc) {
>> +			rc = -EINVAL;
>> +			goto err_exit;
>> +		}
>> +
>> +		rc = asymmetric_verify_common(key, pk, sig, siglen, hash.digest,
>> +					      hash.hdr.length);
>> +	}
>> +
>> +err_exit:
> 
> Normally a label named 'err*' would be preceded by a return.  Here, the label
> "err_exit" is always called, not only when there is an error.  Please rename the
> label to something more appropriate - out, cleanup, etc.

Ok, will call it 'out'.

> 
>> +	key_put(key);
>> +
>> +	return rc;
>>   }
> 
> thanks,
> 
> Mimi
>