| 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: <20251120173157.00007fc4@huawei.com>
Date: Thu, 20 Nov 2025 17:31:57 +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 06/11] coco: guest: arm64: Add support for reading
cached objects from host
On Mon, 17 Nov 2025 19:30:02 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:
> Teach rsi_device_start() to pull the interface report and device
> certificate from the host by querying size, sharing a decrypted buffer
> for the read, copying the payload to private memory. Also track the
> fetched blobs in struct cca_guest_dsc so later stages can hand them to
> the attestation flow.
>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
> ---
> arch/arm64/include/asm/rhi.h | 7 +++
> drivers/virt/coco/arm-cca-guest/rhi-da.c | 80 ++++++++++++++++++++++++
> drivers/virt/coco/arm-cca-guest/rhi-da.h | 1 +
> drivers/virt/coco/arm-cca-guest/rsi-da.h | 8 +++
> 4 files changed, 96 insertions(+)
>
> diff --git a/arch/arm64/include/asm/rhi.h b/arch/arm64/include/asm/rhi.h
> index ce2ed8a440c3..738470dfb869 100644
> --- a/arch/arm64/include/asm/rhi.h
> +++ b/arch/arm64/include/asm/rhi.h
> @@ -39,6 +39,13 @@
> RHI_DA_FEATURE_VDEV_SET_TDI_STATE)
> #define RHI_DA_FEATURES SMC_RHI_CALL(0x004B)
>
> +#define RHI_DA_OBJECT_CERTIFICATE 0x1
> +#define RHI_DA_OBJECT_MEASUREMENT 0x2
> +#define RHI_DA_OBJECT_INTERFACE_REPORT 0x3
> +#define RHI_DA_OBJECT_VCA 0x4
> +#define RHI_DA_OBJECT_SIZE SMC_RHI_CALL(0x004C)
> +#define RHI_DA_OBJECT_READ SMC_RHI_CALL(0x004D)
> +
> #define RHI_DA_VDEV_CONTINUE SMC_RHI_CALL(0x0051)
> #define RHI_DA_VDEV_GET_INTERFACE_REPORT SMC_RHI_CALL(0x0052)
>
> diff --git a/drivers/virt/coco/arm-cca-guest/rhi-da.c b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> index aa17bb3ee562..d29aee0fca58 100644
> --- a/drivers/virt/coco/arm-cca-guest/rhi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rhi-da.c
> @@ -248,3 +248,83 @@ int rhi_update_vdev_measurements_cache(struct pci_dev *pdev,
>
> return ret;
> }
> +
> +int rhi_read_cached_object(int vdev_id, int da_object_type, void **object, int *object_size)
> +{
> + int ret;
> + int max_data_len;
> + struct page *shared_pages;
> + void *data_buf_shared, *data_buf_private;
> + struct rsi_host_call *rhicall;
> +
> + rhicall = kmalloc(sizeof(struct rsi_host_call), GFP_KERNEL);
> + if (!rhicall)
> + return -ENOMEM;
> +
> + rhicall->imm = 0;
> + rhicall->gprs[0] = RHI_DA_OBJECT_SIZE;
> + rhicall->gprs[1] = vdev_id;
> + rhicall->gprs[2] = da_object_type;
> +
> + ret = rsi_host_call(virt_to_phys(rhicall));
> + if (ret != RSI_SUCCESS) {
> + ret = -EIO;
> + goto err_return;
> + }
> +
> + if (rhicall->gprs[0] != RHI_DA_SUCCESS) {
> + ret = -EIO;
> + goto err_return;
> + }
> +
> + /* validate against the max cache object size used on host. */
> + max_data_len = rhicall->gprs[1];
> + if (max_data_len > MAX_CACHE_OBJ_SIZE || max_data_len == 0) {
> + ret = -EIO;
> + goto err_return;
> + }
> + *object_size = max_data_len;
I raise this below, but not sure why this is set way before setting
*object. Can set it later and use max_data_len which I think is
clearer naming anyway.
> +
> + data_buf_private = kmalloc(*object_size, GFP_KERNEL);
> + if (!data_buf_private) {
> + ret = -ENOMEM;
> + goto err_return;
> + }
> +
> + shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, max_data_len);
> + if (!shared_pages) {
> + ret = -ENOMEM;
> + goto err_shared_alloc;
> + }
> + data_buf_shared = page_address(shared_pages);
> +
> + rhicall->imm = 0;
> + rhicall->gprs[0] = RHI_DA_OBJECT_READ;
> + rhicall->gprs[1] = vdev_id;
> + rhicall->gprs[2] = da_object_type;
> + rhicall->gprs[3] = 0; /* offset within the data buffer */
> + rhicall->gprs[4] = max_data_len;
> + rhicall->gprs[5] = virt_to_phys(data_buf_shared);
> + ret = rsi_host_call(virt_to_phys(rhicall));
> + if (ret != RSI_SUCCESS || rhicall->gprs[0] != RHI_DA_SUCCESS) {
> + ret = -EIO;
> + goto err_rhi_call;
> + }
> +
> + memcpy(data_buf_private, data_buf_shared, *object_size);
Given data_buf_private() only seems useful if we aren't in an error
condition, why not move allocation to here and use kmemdup() ?
> + free_shared_pages(shared_pages, max_data_len);
> +
> + *object = data_buf_private;
> + kfree(rhicall);
> + return 0;
> +
> +err_rhi_call:
> + free_shared_pages(shared_pages, max_data_len);
> +err_shared_alloc:
> + kfree(data_buf_private);
> +err_return:
> + *object = NULL;
Is it necessary to zero the passed in variable given this function never
touched it and is returning an error. If it is, can you do that unconditionally
at start of function and override only on success?
> + *object_size = 0;
Likewise for the size - I'm not sure why you set that much earlier
than *object.
With those two gone, this feels like it would be well suited for
some __free magic to handle everything here.
You will need to deal with the free_shared_pages() though which will
require an extra structure definition and helpers to wrap up what
is allocated - similar to what tdx_page_array_alloc does (though without
the bulk aspects of that)
http://lore.kernel.org/all/20251117022311.2443900-7-yilun.xu@linux.intel.com/
Or given the shared page stuff is the inner most aspect anyway you could
just do a helper function from alloc_shared_pages to free_shared_pages
calls so that you can call that free_shared_pages unconditionally before
checking return value.
Note that if you do go with DEFINE_FREE() you 'could' pass in the storage.
I objected to that elsewhere but there is precedence. So have a
struct shared_pages {
struct page *page;
size_t order;
}
define one on the stack and pass it in so that you avoid an extra allocation.
Not a pattern I particularly like though and this isn't expected to be
a particularly fast path so I'd just dynamically allocate a struct shared_pages
inside alloc_shared_pages.
> + kfree(rhicall);
> + return ret;
> +}
Powered by blists - more mailing lists