| 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: <yq5a4iqklzqy.fsf@kernel.org>
Date: Mon, 24 Nov 2025 11:48:45 +0530
From: Aneesh Kumar K.V <aneesh.kumar@...nel.org>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
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
Jonathan Cameron <jonathan.cameron@...wei.com> writes:
> 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 ;)
>
The RHI spec update is still pending. In the meantime, the relevant
details can be found in the RMI spec under section B4.4.69, which
describes the RmiVdevMeasureFlags type. I’ll update the cover letter to
reference the specific spec details that these patches are based on.
>
>> +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.
>
something like
static inline void vdev_meas_params_free(struct rhi_vdev_measurement_params *params)
{
struct page *pages = virt_to_page(params);
free_shared_pages(pages, sizeof(struct rhi_vdev_measurement_params));
}
DEFINE_FREE(vdev_meas_params_free, struct rhi_vdev_measurement_params *, if (_T) vdev_meas_params_free(_T))
>
>> + if (ret) {
>> + pci_err(pdev, "failed to get device measurement (%d)\n", ret);
>> + return ret;
>> + }
>> return 0;
>> }
-aneesh
Powered by blists - more mailing lists