| 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: <20251120152202.00001efb@huawei.com>
Date: Thu, 20 Nov 2025 15:22:02 +0000
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>,
<dan.j.williams@...el.com>, <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>, Bjorn Helgaas <helgaas@...nel.org>, Catalin Marinas
<catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>, Will Deacon
<will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH v2 05/11] coco: guest: arm64: Add support for updating
measurements from device
On Mon, 17 Nov 2025 19:30:01 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:
> Fetch device measurements using RSI_RDEV_GET_MEASUREMENTS. The fetched
> device measurements will be cached in the host.
>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
Hi Aneesh
Minor stuff inline.
thanks
Jonathan
> ---
> arch/arm64/include/asm/rhi.h | 19 ++++++++
> drivers/virt/coco/arm-cca-guest/rhi-da.c | 48 ++++++++++++++++++++
> drivers/virt/coco/arm-cca-guest/rhi-da.h | 2 +
> drivers/virt/coco/arm-cca-guest/rsi-da.c | 58 ++++++++++++++++++++++++
> drivers/virt/coco/arm-cca-guest/rsi-da.h | 2 +
> 5 files changed, 129 insertions(+)
>
> diff --git a/arch/arm64/include/asm/rhi.h b/arch/arm64/include/asm/rhi.h
> index 5f140015afc3..ce2ed8a440c3 100644
> --- a/arch/arm64/include/asm/rhi.h
> +++ b/arch/arm64/include/asm/rhi.h
> @@ -42,6 +42,25 @@
> #define RHI_DA_VDEV_CONTINUE SMC_RHI_CALL(0x0051)
> #define RHI_DA_VDEV_GET_INTERFACE_REPORT SMC_RHI_CALL(0x0052)
>
> +#define RHI_VDEV_MEASURE_SIGNED BIT(0)
> +#define RHI_VDEV_MEASURE_RAW BIT(1)
> +#define RHI_VDEV_MEASURE_EXCHANGE BIT(2)
Whilst I appreciate the specs are still subject to minor changes,
it would be very helpful if definitions like the ones above were
all accompanied by a spec reference.
Which is another way of saying I can't find these ones ;)
> +struct rhi_vdev_measurement_params {
> + union {
> + u64 flags;
> + u8 padding0[256];
> + };
> + union {
> + u8 indices[32];
> + u8 padding1[256];
> + };
> + union {
> + u8 nonce[32];
> + u8 padding2[256];
> + };
> +};
> +#define RHI_DA_VDEV_GET_MEASUREMENTS SMC_RHI_CALL(0x0053)
> +
> #define RHI_DA_TDI_CONFIG_UNLOCKED 0x0
> #define RHI_DA_TDI_CONFIG_LOCKED 0x1
> #define RHI_DA_TDI_CONFIG_RUN 0x2
> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.c b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> index f4fb8577e1b5..aa17bb3ee562 100644
> --- a/drivers/virt/coco/arm-cca-guest/rhi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> @@ -200,3 +200,51 @@ int rhi_update_vdev_interface_report_cache(struct pci_dev *pdev)
>
> return ret;
> }
> +
> +static inline int rhi_vdev_get_measurements(unsigned long vdev_id,
> + phys_addr_t vdev_meas_phys,
> + unsigned long *cookie)
> +{
> + unsigned long ret;
> +
> + struct rsi_host_call *rhi_call __free(kfree) =
> + kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);
sizeof(*rhi_call) slightly preferred.
> + if (!rhi_call)
> + return -ENOMEM;
> +
> + rhi_call->imm = 0;
> + rhi_call->gprs[0] = RHI_DA_VDEV_GET_MEASUREMENTS;
> + rhi_call->gprs[1] = vdev_id;
> + rhi_call->gprs[2] = vdev_meas_phys;
> +
> + ret = rsi_host_call(virt_to_phys(rhi_call));
> + if (ret != RSI_SUCCESS)
> + return -EIO;
> +
> + *cookie = rhi_call->gprs[1];
> + return map_rhi_da_error(rhi_call->gprs[0]);
> +}
> +
> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.c b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> index c8ba72e4be3e..aa6e13e4c0ea 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> @@ -4,6 +4,7 @@
> */
>
> #include <linux/pci.h>
> +#include <linux/mem_encrypt.h>
> #include <asm/rsi_cmds.h>
>
> #include "rsi-da.h"
> @@ -35,9 +36,50 @@ int cca_device_unlock(struct pci_dev *pdev)
> return 0;
> }
>
> +struct page *alloc_shared_pages(int nid, gfp_t gfp_mask, unsigned long min_size)
> +{
> + int ret;
> + struct page *page;
> + /* We should normalize the size based on hypervisor page size */
> + int page_order = get_order(min_size);
> +
> + /* Always request for zero filled pages */
Not sure the comment is necessary given the visible flag.
If you were saying 'why' then a comment would be fine, but this is just
repeating what we can see in the code.
> + page = alloc_pages_node(nid, gfp_mask | __GFP_ZERO, page_order);
> +
> + if (!page)
> + return NULL;
> +
> + ret = set_memory_decrypted((unsigned long)page_address(page),
> + 1 << page_order);
> + /*
> + * If set_memory_decrypted() fails then we don't know what state the
> + * page is in, so we can't free it. Instead we leak it.
> + * set_memory_decrypted() will already have WARNed.
> + */
> + if (ret)
> + return NULL;
> +
> + return page;
> +}
> +
> +int free_shared_pages(struct page *page, unsigned long min_size)
> +{
> + int ret;
> + /* We should normalize the size based on hypervisor page size */
> + int page_order = get_order(min_size);
> +
> + ret = set_memory_encrypted((unsigned long)page_address(page), 1 << page_order);
> + /* If we fail to mark it encrypted don't free it back */
> + if (!ret)
> + __free_pages(page, page_order);
> + return ret;
If failing to mark it encrypted is an error I'd find it easier to read this if it were
out of line.
ret = set_memory...
if (ret)
return ret;
__free_pages();
return 0;
This is just a preference as someone who reads a lot of code. Having error handling
as the out of line path is more common and so what my brain (and other reviewers)
has long been trained to expect.
> +}
> +
> int cca_update_device_object_cache(struct pci_dev *pdev, struct cca_guest_dsc *dsc)
> {
> int ret;
> + struct page *shared_pages;
> + struct rhi_vdev_measurement_params *dev_meas;
>
> ret = rhi_update_vdev_interface_report_cache(pdev);
> if (ret) {
> @@ -45,5 +87,21 @@ int cca_update_device_object_cache(struct pci_dev *pdev, struct cca_guest_dsc *d
> return ret;
> }
>
> + shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, sizeof(struct rhi_vdev_measurement_params));
Perhaps sizeof(*dev_meas) would be both shorter and clearer.
> + if (!shared_pages)
> + return -ENOMEM;
> +
> + dev_meas = (struct rhi_vdev_measurement_params *)page_address(shared_pages);
> + /* request for signed full transcript */
> + dev_meas->flags = RHI_VDEV_MEASURE_SIGNED | RHI_VDEV_MEASURE_EXCHANGE;
> + /* request all measurement block. Set bit 254 */
> + dev_meas->indices[31] = 0x40;
> + ret = rhi_update_vdev_measurements_cache(pdev, dev_meas);
> +
> + free_shared_pages(shared_pages, sizeof(struct rhi_vdev_measurement_params));
It might be worth appropriate DEFINE_FREE() magic to to stash the size away
and simplify this a tiny bit. Kind of depends on whether this is a one off
or those helpers are going to get a reasonable amount of use.
> + if (ret) {
> + pci_err(pdev, "failed to get device measurement (%d)\n", ret);
> + return ret;
> + }
> return 0;
> }
Powered by blists - more mailing lists