Re: [PATCH RESEND v2 12/12] coco: host: arm64: Register device public key with RMM

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 27 Oct 2025 15:26:02 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:

> - Introduce the SMC_RMI_PDEV_SET_PUBKEY helper and the associated struct
> rmi_public_key_params so the host can hand the device’s public key to
> the RMM.
> 
> - Parse the certificate chain cached during IDE setup, extract the final
> certificate’s public key, and recognise RSA-3072, ECDSA-P256, and
> ECDSA-P384 keys before calling into the RMM.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

Various comments inline.

Overall this patch set seems to be coming together nicely to me.

Jonathan

> diff --git a/drivers/virt/coco/arm-cca-host/rmi-da.c b/drivers/virt/coco/arm-cca-host/rmi-da.c
> index 644609618a7a..c9780ca64c17 100644
> --- a/drivers/virt/coco/arm-cca-host/rmi-da.c
> +++ b/drivers/virt/coco/arm-cca-host/rmi-da.c
> @@ -8,6 +8,9 @@
>  #include <linux/pci-doe.h>
>  #include <linux/delay.h>
>  #include <asm/rmi_cmds.h>
> +#include <crypto/internal/rsa.h>
> +#include <keys/asymmetric-type.h>
> +#include <keys/x509-parser.h>
>  
>  #include "rmi-da.h"
>  
> @@ -361,6 +364,146 @@ static int do_pdev_communicate(struct pci_tsm *tsm, enum rmi_pdev_state target_s
>  	return do_dev_communicate(PDEV_COMMUNICATE, tsm, target_state, RMI_PDEV_ERROR);
>  }
>  
> +static int parse_certificate_chain(struct pci_tsm *tsm)
> +{
> +	struct cca_host_pf0_dsc *pf0_dsc;
> +	unsigned int chain_size;
> +	unsigned int offset = 0;
> +	u8 *chain_data;
> +
> +	pf0_dsc = to_cca_pf0_dsc(tsm->pdev);
> +
> +	/* If device communication didn't results in certificate caching. */
> +	if (!pf0_dsc->cert_chain.cache || !pf0_dsc->cert_chain.cache->offset)
> +		return -EINVAL;
> +
> +	chain_size = pf0_dsc->cert_chain.cache->offset;
> +	chain_data = pf0_dsc->cert_chain.cache->buf;
> +
> +	while (offset < chain_size) {
> +		ssize_t cert_len =
> +			x509_get_certificate_length(chain_data + offset,
> +						    chain_size - offset);
> +		if (cert_len < 0)
> +			return cert_len;
> +
> +		struct x509_certificate *cert __free(x509_free_certificate) =
> +			x509_cert_parse(chain_data + offset, cert_len);
> +
> +		if (IS_ERR(cert)) {
> +			pci_warn(tsm->pdev, "parsing of certificate chain not successful\n");
> +			return PTR_ERR(cert);
> +		}
> +
> +		/* The key in the last cert in the chain is used */
> +		if (offset + cert_len == chain_size) {
> +			pf0_dsc->cert_chain.public_key = kzalloc(cert->pub->keylen, GFP_KERNEL);
I'd use a local variable for this + __free(kfree)
Then assign with no_free_ptr()
> +			if (!pf0_dsc->cert_chain.public_key)
> +				return -ENOMEM;
> +
> +			if (!strcmp("ecdsa-nist-p256", cert->pub->pkey_algo)) {
> +				pf0_dsc->rmi_signature_algorithm = RMI_SIG_ECDSA_P256;
> +			} else if (!strcmp("ecdsa-nist-p384", cert->pub->pkey_algo)) {
> +				pf0_dsc->rmi_signature_algorithm = RMI_SIG_ECDSA_P384;
> +			} else if (!strcmp("rsa", cert->pub->pkey_algo)) {
> +				pf0_dsc->rmi_signature_algorithm = RMI_SIG_RSASSA_3072;
> +			} else {
> +				kfree(pf0_dsc->cert_chain.public_key);
> +				pf0_dsc->cert_chain.public_key = NULL;
Set it only when succeeded (local variable until then). 
> +				return -ENXIO;
> +			}
> +			memcpy(pf0_dsc->cert_chain.public_key, cert->pub->key, cert->pub->keylen);
> +			pf0_dsc->cert_chain.public_key_size = cert->pub->keylen;
I think at this point we know we are at end of cert chain?  Break would make that obvious.

> +		}
> +
> +		offset += cert_len;
> +	}
> +
> +	pf0_dsc->cert_chain.valid = true;
> +	return 0;
> +}
> +
> +DEFINE_FREE(free_page, unsigned long, if (_T) free_page(_T))

Fully agree with Jason on this one. If it make sense
belongs in appropriate header.
I'm a bit bothered by types though as the parameter is IIRC an unsigned long.

Might need some wrappers to deal with casting.  To me feels likely
to be controversial so pitch it separately from this series.

If you want a define free here create a local helper function tightly
scoped to the type you use it for below.

Or just wrap up the guts of the code in a helper function and
unconditionally free it the old fashioned way.


> +static int pdev_set_public_key(struct pci_tsm *tsm)
> +{
> +	unsigned long expected_key_len;
> +	struct cca_host_pf0_dsc *pf0_dsc;
> +	int ret;
> +
> +	pf0_dsc = to_cca_pf0_dsc(tsm->pdev);
> +	/* Check that all the necessary information was captured from communication */
> +	if (!pf0_dsc->cert_chain.valid)
> +		return -EINVAL;
> +
> +	struct rmi_public_key_params *key_params __free(free_page) =
> +		(struct rmi_public_key_params *)get_zeroed_page(GFP_KERNEL);
> +	if (!key_params)
> +		return -ENOMEM;
> +
> +	key_params->rmi_signature_algorithm = pf0_dsc->rmi_signature_algorithm;
> +
> +	switch (key_params->rmi_signature_algorithm) {
> +	case RMI_SIG_ECDSA_P384:
> +	{
> +		expected_key_len = 97;
That feels like it should be a define somewhere.
> +
> +		if (pf0_dsc->cert_chain.public_key_size != expected_key_len)
> +			return -EINVAL;
> +
> +		key_params->public_key_len = pf0_dsc->cert_chain.public_key_size;
> +		memcpy(key_params->public_key,
> +		       pf0_dsc->cert_chain.public_key,
> +		       pf0_dsc->cert_chain.public_key_size);
> +		key_params->metadata_len = 0;
> +		break;
> +	}
> +	case RMI_SIG_ECDSA_P256:
> +	{
> +		expected_key_len = 65;
Same with this constant.

> +
> +		if (pf0_dsc->cert_chain.public_key_size != expected_key_len)
> +			return -EINVAL;
> +
> +		key_params->public_key_len = pf0_dsc->cert_chain.public_key_size;
> +		memcpy(key_params->public_key,
> +		       pf0_dsc->cert_chain.public_key,
> +		       pf0_dsc->cert_chain.public_key_size);
> +		key_params->metadata_len = 0;
> +		break;
> +	}
> +	case RMI_SIG_RSASSA_3072:
> +	{
> +		struct rsa_key rsa_key = {0};
> +
> +		expected_key_len = 385;
And this one ;)
> +		int ret_rsa_parse = rsa_parse_pub_key(&rsa_key,
> +						      pf0_dsc->cert_chain.public_key,
> +						      pf0_dsc->cert_chain.public_key_size);
Don't mix declarations and code except for cleanup.h stuff.

> +		/* This also checks the key_len */
> +		if (ret_rsa_parse)
> +			return ret_rsa_parse;
> +		/*
> +		 * exponent is usually 65537 (size = 24bits) but in rare cases
> +		 * the size can be as large as the modulus
> +		 */
> +		if (rsa_key.e_sz > expected_key_len)
> +			return -EINVAL;
> +
> +		key_params->public_key_len = rsa_key.n_sz;
> +		key_params->metadata_len = rsa_key.e_sz;
> +		memcpy(key_params->public_key, rsa_key.n, rsa_key.n_sz);
> +		memcpy(key_params->metadata, rsa_key.e, rsa_key.e_sz);
> +		break;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = rmi_pdev_set_pubkey(virt_to_phys(pf0_dsc->rmm_pdev),
> +				  virt_to_phys(key_params));
> +	return ret;

return rmi_pdev_set_pubkey();

> +}
> +
>  void pdev_communicate_work(struct work_struct *work)
>  {
>  	unsigned long state;
> @@ -410,7 +553,28 @@ static int submit_pdev_comm_work(struct pci_dev *pdev, int target_state)
>  
>  int pdev_ide_setup(struct pci_dev *pdev)
>  {
> -	return submit_pdev_comm_work(pdev, RMI_PDEV_NEEDS_KEY);
> +	int ret;
> +
> +	ret = submit_pdev_comm_work(pdev, RMI_PDEV_NEEDS_KEY);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * we now have certificate chain in dsm->cert_chain. Parse

Wrap at 80 chars. This is a bit short.

> +	 * that and set the pubkey.
> +	 */
> +	ret = parse_certificate_chain(pdev->tsm);
> +	if (ret)
> +		return ret;
> +
> +	ret = pdev_set_public_key(pdev->tsm);
> +	if (ret)
> +		return ret;
> +
> +	ret = submit_pdev_comm_work(pdev, RMI_PDEV_READY);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return submit_pdev_comm_work(...)

>  }