Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> writes:
> On Mon, 28 Jul 2025 19:22:15 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@xxxxxxxxxx> wrote:
>
...
>> + 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.
>
yes, struct rsi_device_info is 512 bytes.
>
>> + 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?
>
yes. But won't that be confusing? Is there a difference?
Also struct dsm_device_info is not same as struct rsi_device_info. We
don't need to keep all that padding in dsm_device_info.
>
>> + 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)
-aneesh
