| 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: <yq5a1plnncqf.fsf@kernel.org>
Date: Mon, 24 Nov 2025 12:22:56 +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 06/11] coco: guest: arm64: Add support for reading
cached objects from host
Jonathan Cameron <jonathan.cameron@...wei.com> writes:
> 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;
>> +}
I'm not sure we need a new struct here, since other shared pages do have
type like struct rhi_vdev_measurement_params. The read-cached object is
the only exception.
That said, I’ve updated the other allocation to use __free() as suggested.
int rhi_read_cached_object(int vdev_id, int da_object_type, void **object, int *object_size)
{
int ret;
int max_data_len;
void *data_buf_shared;
struct page *shared_pages;
*object_size = 0;
*object = NULL;
struct rsi_host_call *rhicall __free(kfree) =
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)
return -EIO;
if (rhicall->gprs[0] != RHI_DA_SUCCESS)
return -EIO;
/* 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)
return -EIO;
shared_pages = alloc_shared_pages(NUMA_NO_NODE, GFP_KERNEL, max_data_len);
if (!shared_pages)
return -ENOMEM;
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) {
free_shared_pages(shared_pages, max_data_len);
return -EIO;
}
void *data_buf_private __free(kvfree) =
kvmemdup(data_buf_shared, max_data_len, GFP_KERNEL);
/* free the shared pages irrespective of error condition */
free_shared_pages(shared_pages, max_data_len);
if (!data_buf_private)
return -ENOMEM;
*object = data_buf_private;
*object_size = max_data_len;
return 0;
}
Powered by blists - more mailing lists