| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250731113653.000000cd@huawei.com>
Date: Thu, 31 Jul 2025 11:36:53 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org>
CC: <linux-coco@...ts.linux.dev>, <kvmarm@...ts.linux.dev>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <aik@....com>,
<lukas@...ner.de>, Samuel Ortiz <sameo@...osinc.com>, Xu Yilun
<yilun.xu@...ux.intel.com>, Jason Gunthorpe <jgg@...pe.ca>, "Suzuki K
Poulose" <Suzuki.Poulose@....com>, Steven Price <steven.price@....com>,
Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
Will Deacon <will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [RFC PATCH v1 38/38] coco: guest: arm64: Add support for
fetching device info
On Mon, 28 Jul 2025 19:22:15 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:
> RSI_RDEV_GET_INFO returns different digest hash values, which can be
> compared with host cached values to ensure the host didn't tamper with
> the cached data.
>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
Hi Aneesh
A few comments on this one
Jonathan
> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.c b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> index 6222b10964ee..a1bb225adb4c 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> @@ -6,6 +6,7 @@
> #include <linux/pci.h>
> #include <linux/mem_encrypt.h>
> #include <asm/rsi_cmds.h>
> +#include <crypto/hash.h>
>
> #include "rsi-da.h"
>
> @@ -186,11 +187,102 @@ rsi_rdev_get_measurements(struct pci_dev *pdev, unsigned long vdev_id,
> return RSI_SUCCESS;
> }
>
> +static int verify_digests(struct cca_guest_dsc *dsm)
> +{
> + int i;
> + int ret;
> + u8 digest[SHA512_DIGEST_SIZE];
> + int sdesc_size;
> + size_t digest_size;
> + char *hash_alg_name;
> + struct shash_desc *shash;
> + struct crypto_shash *alg;
> + struct pci_dev *pdev = dsm->pci.tsm.pdev;
> + struct {
> + uint8_t *report;
> + size_t size;
> + uint8_t *digest;
> + } reports[] = {
> + {
> + dsm->interface_report,
> + dsm->interface_report_size,
> + dsm->dev_info.report_digest
> + },
> + {
> + dsm->certificate,
> + dsm->certificate_size,
> + dsm->dev_info.cert_digest
> + },
> + {
> + dsm->measurements,
> + dsm->measurements_size,
> + dsm->dev_info.meas_digest
> + }
> + };
> +
> +
> + if (dsm->dev_info.hash_algo == RSI_HASH_SHA_256) {
This to me smells like a place that will need switch sooner or
later, maybe just do it now.
> + hash_alg_name = "sha256";
> + digest_size = SHA256_DIGEST_SIZE;
> + } else if (dsm->dev_info.hash_algo == RSI_HASH_SHA_512) {
> + hash_alg_name = "sha512";
> + digest_size = SHA512_DIGEST_SIZE;
> + } else {
> + pci_err(pdev, "unknown realm hash algorithm!\n");
> + ret = -EINVAL;
return -EINVAL;
> + goto err_out;
> + }
> +
> + alg = crypto_alloc_shash(hash_alg_name, 0, 0);
As below - I'd spin a DEFINE_FREE() for this to simplify error
paths in here and remove the labels that had me confused briefly.
> + if (IS_ERR(alg)) {
> + pci_err(pdev, "cannot allocate %s\n", hash_alg_name);
> + return PTR_ERR(alg);
> + }
> +
> + sdesc_size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
Not common in crypto so perhaps just leave this as you have it.
sdesc_size = struct_size(struct shash_dec, __ctx, crypto_shash_desc_size(alg));
is more informative on what is going on here.
> + shash = kzalloc(sdesc_size, GFP_KERNEL);
> + if (!shash) {
> + pci_err(pdev, "cannot allocate sdesc\n");
> + ret = -ENOMEM;
> + goto err_free_shash;
> + }
> + shash->tfm = alg;
> +
> + for (i = 0; i < ARRAY_SIZE(reports); i++) {
> + ret = crypto_shash_digest(shash, reports[i].report,
> + reports[i].size, digest);
To me a bit marginal on whether this loop and the structures above
are beneficial over straight line code.
> + if (ret) {
> + pci_err(pdev, "failed to compute digest, %d\n", ret);
> + goto err_free_sdesc;
> + }
> +
> + if (memcmp(reports[i].digest, digest, digest_size)) {
> + pci_err(pdev, "invalid digest\n");
> + ret = -EINVAL;
> + goto err_free_sdesc;
> + }
> + }
> +
> + kfree(shash);
> + crypto_free_shash(alg);
> +
> + pci_info(pdev, "Successfully verified the digests\n");
debug.
> + return 0;
> +
> +err_free_sdesc:
I'd tweak these labels if you keep them. This isn't freeing the sdesc.
> + kfree(shash);
Looks perfect for some __free() magic dust.
> +err_free_shash:
> + crypto_free_shash(alg);
DEFINE_FREE() needed for this but looks pretty uncontroversial.
> +err_out:
As below. I'd not do this.
> + return ret;
> +}
> +
> int rsi_device_lock(struct pci_dev *pdev)
> {
> unsigned long ret;
> unsigned long tdisp_version;
> struct rsi_device_measurements_params *rsi_dev_meas;
> + struct rsi_device_info *dev_info;
> struct cca_guest_dsc *dsm = to_cca_guest_dsc(pdev);
> int vdev_id = (pci_domain_nr(pdev->bus) << 16) |
> PCI_DEVID(pdev->bus->number, pdev->devfn);
> @@ -252,8 +344,44 @@ int rsi_device_lock(struct pci_dev *pdev)
> return -EIO;
> }
>
> + /* RMM expects sizeof(dev_info) (512 bytes) aligned address */
> + dev_info = kmalloc(sizeof(*dev_info), GFP_KERNEL);
Use a __free(kfree) here (and direct returns on errors) given it's freed
in all paths and we don't care if it is freed before or after verifying the digests.
I'm being slow today, but what in that enforces the alignment? I guess
it's that the structure happens to be big enough that it happens naturally?
I'd allocate max(512, sizeof(*dev_info)) to make it explicitly the case.
> + if (!dev_info) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> + ret = rsi_rdev_get_info(vdev_id, dsm->instance_id, virt_to_phys(dev_info));
> + if (ret != RSI_SUCCESS) {
> + pci_err(pdev, "failed to get device digests (%lu)\n", ret);
> + ret = -EIO;
> + kfree(dev_info);
> + goto err_out;
> + }
> +
> + dsm->dev_info.attest_type = dev_info->attest_type;
> + dsm->dev_info.cert_id = dev_info->cert_id;
> + dsm->dev_info.hash_algo = dev_info->hash_algo;
> + memcpy(dsm->dev_info.cert_digest, dev_info->cert_digest, SHA512_DIGEST_SIZE);
> + memcpy(dsm->dev_info.meas_digest, dev_info->meas_digest, SHA512_DIGEST_SIZE);
> + memcpy(dsm->dev_info.report_digest, dev_info->report_digest, SHA512_DIGEST_SIZE);
> +
Can't you memcpy the whole thing in one go?
> + kfree(dev_info);
> + /*
> + * Verify that the digests of the provided reports match with the
> + * digests from RMM
> + */
> + ret = verify_digests(dsm);
> + if (ret) {
> + pci_err(pdev, "device digest validation failed (%ld)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +err_out:
I'll always grumble about these. To me it always makes the code
less readable. Some others disagree though ;(
> return ret;
> }
> +
Looks like this should have been in an earlier patch.
> static inline unsigned long rsi_rdev_start(struct pci_dev *pdev,
> unsigned long vdev_id, unsigned long inst_id)
> {
> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.h b/drivers/virt/coco/arm-cca-guest/rsi-da.h
> index f26156d9be81..e8953b8e85a3 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.h
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.h
> @@ -10,6 +10,7 @@
> #include <linux/pci-tsm.h>
> #include <asm/rsi_smc.h>
> #include <asm/rhi.h>
> +#include <crypto/sha2.h>
>
> struct pci_tdisp_device_interface_report {
> u16 interface_info;
> @@ -33,6 +34,17 @@ struct pci_tdisp_mmio_range {
> #define TSM_INTF_REPORT_MMIO_RESERVED GENMASK(15, 4)
> #define TSM_INTF_REPORT_MMIO_RANGE_ID GENMASK(31, 16)
>
> +struct dsm_device_info {
> + u64 flags;
> + u64 attest_type;
> + u64 cert_id;
> + u64 hash_algo;
> + u8 cert_digest[SHA512_DIGEST_SIZE];
> + u8 meas_digest[SHA512_DIGEST_SIZE];
> + u8 report_digest[SHA512_DIGEST_SIZE];
> +};
> +
One probably enough.
> +
> struct cca_guest_dsc {
> struct pci_tsm_pf0 pci;
> unsigned long instance_id;
> @@ -42,6 +54,7 @@ struct cca_guest_dsc {
> int certificate_size;
> void *measurements;
> int measurements_size;
> + struct dsm_device_info dev_info;
> };
>
> static inline struct cca_guest_dsc *to_cca_guest_dsc(struct pci_dev *pdev)
Powered by blists - more mailing lists