From: Sami Mujawar <sami.mujawar@arm.com>
Introduce an arm-cca-guest driver that registers with
the configfs-tsm module to provide user interfaces for
retrieving an attestation token.
When a new report is requested the arm-cca-guest driver
invokes the appropriate RSI interfaces to query an
attestation token.
The steps to retrieve an attestation token are as follows:
1. Mount the configfs filesystem if not already mounted
mount -t configfs none /sys/kernel/config
2. Generate an attestation token
report=/sys/kernel/config/tsm/report/report0
mkdir $report
dd if=/dev/urandom bs=64 count=1 > $report/inblob
hexdump -C $report/outblob
rmdir $report
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
v3: Minor improvements to comments and adapt to the renaming of
GRANULE_SIZE to RSI_GRANULE_SIZE.
---
drivers/virt/coco/Kconfig | 2 +
drivers/virt/coco/Makefile | 1 +
drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
drivers/virt/coco/arm-cca-guest/Makefile | 2 +
.../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++++++++
5 files changed, 227 insertions(+)
create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index d9ff676bf48d..ff869d883d95 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -14,3 +14,5 @@ source "drivers/virt/coco/pkvm-guest/Kconfig"
source "drivers/virt/coco/sev-guest/Kconfig"
source "drivers/virt/coco/tdx-guest/Kconfig"
+
+source "drivers/virt/coco/arm-cca-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index b69c30c1c720..c3d07cfc087e 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_EFI_SECRET) += efi_secret/
obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
obj-$(CONFIG_SEV_GUEST) += sev-guest/
obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
+obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
new file mode 100644
index 000000000000..9dd27c3ee215
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-guest/Kconfig
@@ -0,0 +1,11 @@
+config ARM_CCA_GUEST
+ tristate "Arm CCA Guest driver"
+ depends on ARM64
+ default m
+ select TSM_REPORTS
+ help
+ The driver provides userspace interface to request and
+ attestation report from the Realm Management Monitor(RMM).
+
+ If you choose 'M' here, this module will be called
+ arm-cca-guest.
diff --git a/drivers/virt/coco/arm-cca-guest/Makefile b/drivers/virt/coco/arm-cca-guest/Makefile
new file mode 100644
index 000000000000..69eeba08e98a
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-guest/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest.o
diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
new file mode 100644
index 000000000000..e22a565cb425
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 ARM Ltd.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/cc_platform.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/smp.h>
+#include <linux/tsm.h>
+#include <linux/types.h>
+
+#include <asm/rsi.h>
+
+/**
+ * struct arm_cca_token_info - a descriptor for the token buffer.
+ * @granule: PA of the page to which the token will be written
+ * @offset: Offset within granule to start of buffer in bytes
+ * @len: Number of bytes of token data that was retrieved
+ * @result: result of rsi_attestation_token_continue operation
+ */
+struct arm_cca_token_info {
+ phys_addr_t granule;
+ unsigned long offset;
+ int result;
+};
+
+/**
+ * arm_cca_attestation_continue - Retrieve the attestation token data.
+ *
+ * @param: pointer to the arm_cca_token_info
+ *
+ * Attestation token generation is a long running operation and therefore
+ * the token data may not be retrieved in a single call. Moreover, the
+ * token retrieval operation must be requested on the same CPU on which the
+ * attestation token generation was initialised.
+ * This helper function is therefore scheduled on the same CPU multiple
+ * times until the entire token data is retrieved.
+ */
+static void arm_cca_attestation_continue(void *param)
+{
+ unsigned long len;
+ unsigned long size;
+ struct arm_cca_token_info *info;
+
+ if (!param)
+ return;
+
+ info = (struct arm_cca_token_info *)param;
+
+ size = RSI_GRANULE_SIZE - info->offset;
+ info->result = rsi_attestation_token_continue(info->granule,
+ info->offset, size, &len);
+ info->offset += len;
+}
+
+/**
+ * arm_cca_report_new - Generate a new attestation token.
+ *
+ * @report: pointer to the TSM report context information.
+ * @data: pointer to the context specific data for this module.
+ *
+ * Initialise the attestation token generation using the challenge data
+ * passed in the TSM descriptor. Allocate memory for the attestation token
+ * and schedule calls to retrieve the attestation token on the same CPU
+ * on which the attestation token generation was initialised.
+ *
+ * The challenge data must be at least 32 bytes and no more than 64 bytes. If
+ * less than 64 bytes are provided it will be zero padded to 64 bytes.
+ *
+ * Return:
+ * * %0 - Attestation token generated successfully.
+ * * %-EINVAL - A parameter was not valid.
+ * * %-ENOMEM - Out of memory.
+ * * %-EFAULT - Failed to get IPA for memory page(s).
+ * * A negative status code as returned by smp_call_function_single().
+ */
+static int arm_cca_report_new(struct tsm_report *report, void *data)
+{
+ int ret;
+ int cpu;
+ long max_size;
+ unsigned long token_size;
+ struct arm_cca_token_info info;
+ void *buf;
+ u8 *token __free(kvfree) = NULL;
+ struct tsm_desc *desc = &report->desc;
+
+ if (!report)
+ return -EINVAL;
+
+ if (desc->inblob_len < 32 || desc->inblob_len > 64)
+ return -EINVAL;
+
+ /*
+ * Get a CPU on which the attestation token generation will be
+ * scheduled and initialise the attestation token generation.
+ */
+ cpu = get_cpu();
+ max_size = rsi_attestation_token_init(desc->inblob, desc->inblob_len);
+ put_cpu();
+
+ if (max_size <= 0)
+ return -EINVAL;
+
+ /* Allocate outblob */
+ token = kvzalloc(max_size, GFP_KERNEL);
+ if (!token)
+ return -ENOMEM;
+
+ /*
+ * Since the outblob may not be physically contiguous, use a page
+ * to bounce the buffer from RMM.
+ */
+ buf = alloc_pages_exact(RSI_GRANULE_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* Get the PA of the memory page(s) that were allocated. */
+ info.granule = (unsigned long)virt_to_phys(buf);
+
+ token_size = 0;
+ /* Loop until the token is ready or there is an error. */
+ do {
+ /* Retrieve one RSI_GRANULE_SIZE data per loop iteration. */
+ info.offset = 0;
+ do {
+ /*
+ * Schedule a call to retrieve a sub-granule chunk
+ * of data per loop iteration.
+ */
+ ret = smp_call_function_single(cpu,
+ arm_cca_attestation_continue,
+ (void *)&info, true);
+ if (ret != 0) {
+ token_size = 0;
+ goto exit_free_granule_page;
+ }
+
+ ret = info.result;
+ } while ((ret == RSI_INCOMPLETE) &&
+ (info.offset < RSI_GRANULE_SIZE));
+
+ /*
+ * Copy the retrieved token data from the granule
+ * to the token buffer, ensuring that the RMM doesn't
+ * overflow the buffer.
+ */
+ if (WARN_ON(token_size + info.offset > max_size))
+ break;
+ memcpy(&token[token_size], buf, info.offset);
+ token_size += info.offset;
+ } while (ret == RSI_INCOMPLETE);
+
+ if (ret != RSI_SUCCESS) {
+ ret = -ENXIO;
+ token_size = 0;
+ goto exit_free_granule_page;
+ }
+
+ report->outblob = no_free_ptr(token);
+exit_free_granule_page:
+ report->outblob_len = token_size;
+ free_pages_exact(buf, RSI_GRANULE_SIZE);
+ return ret;
+}
+
+static const struct tsm_ops arm_cca_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .report_new = arm_cca_report_new,
+};
+
+/**
+ * arm_cca_guest_init - Register with the Trusted Security Module (TSM)
+ * interface.
+ *
+ * Return:
+ * * %0 - Registered successfully with the TSM interface.
+ * * %-ENODEV - The execution context is not an Arm Realm.
+ * * %-EINVAL - A parameter was not valid.
+ * * %-EBUSY - Already registered.
+ */
+static int __init arm_cca_guest_init(void)
+{
+ int ret;
+
+ if (!is_realm_world())
+ return -ENODEV;
+
+ ret = tsm_register(&arm_cca_tsm_ops, NULL);
+ if (ret < 0)
+ pr_err("Failed to register with TSM.\n");
+
+ return ret;
+}
+module_init(arm_cca_guest_init);
+
+/**
+ * arm_cca_guest_exit - unregister with the Trusted Security Module (TSM)
+ * interface.
+ */
+static void __exit arm_cca_guest_exit(void)
+{
+ tsm_unregister(&arm_cca_tsm_ops);
+}
+module_exit(arm_cca_guest_exit);
+
+MODULE_AUTHOR("Sami Mujawar <sami.mujawar@arm.com>");
+MODULE_DESCRIPTION("Arm CCA Guest TSM Driver.");
+MODULE_LICENSE("GPL");
--
2.34.1On 10/5/24 12:43 AM, Steven Price wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
>
> Introduce an arm-cca-guest driver that registers with
> the configfs-tsm module to provide user interfaces for
> retrieving an attestation token.
>
> When a new report is requested the arm-cca-guest driver
> invokes the appropriate RSI interfaces to query an
> attestation token.
>
> The steps to retrieve an attestation token are as follows:
> 1. Mount the configfs filesystem if not already mounted
> mount -t configfs none /sys/kernel/config
> 2. Generate an attestation token
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> dd if=/dev/urandom bs=64 count=1 > $report/inblob
> hexdump -C $report/outblob
> rmdir $report
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> v3: Minor improvements to comments and adapt to the renaming of
> GRANULE_SIZE to RSI_GRANULE_SIZE.
> ---
> drivers/virt/coco/Kconfig | 2 +
> drivers/virt/coco/Makefile | 1 +
> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++++++++
> 5 files changed, 227 insertions(+)
> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index d9ff676bf48d..ff869d883d95 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -14,3 +14,5 @@ source "drivers/virt/coco/pkvm-guest/Kconfig"
> source "drivers/virt/coco/sev-guest/Kconfig"
>
> source "drivers/virt/coco/tdx-guest/Kconfig"
> +
> +source "drivers/virt/coco/arm-cca-guest/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index b69c30c1c720..c3d07cfc087e 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_EFI_SECRET) += efi_secret/
> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
> obj-$(CONFIG_SEV_GUEST) += sev-guest/
> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
> new file mode 100644
> index 000000000000..9dd27c3ee215
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
> @@ -0,0 +1,11 @@
> +config ARM_CCA_GUEST
> + tristate "Arm CCA Guest driver"
> + depends on ARM64
> + default m
> + select TSM_REPORTS
> + help
> + The driver provides userspace interface to request and
> + attestation report from the Realm Management Monitor(RMM).
> +
> + If you choose 'M' here, this module will be called
> + arm-cca-guest.
> diff --git a/drivers/virt/coco/arm-cca-guest/Makefile b/drivers/virt/coco/arm-cca-guest/Makefile
> new file mode 100644
> index 000000000000..69eeba08e98a
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest.o
> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> new file mode 100644
> index 000000000000..e22a565cb425
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 ARM Ltd.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/cc_platform.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/smp.h>
> +#include <linux/tsm.h>
> +#include <linux/types.h>
> +
> +#include <asm/rsi.h>
> +
> +/**
> + * struct arm_cca_token_info - a descriptor for the token buffer.
> + * @granule: PA of the page to which the token will be written
^^^^^^^^
s/the page/the granule. They are same thing when we have 4KB page size,
but there are conceptually different if I'm correct.
> + * @offset: Offset within granule to start of buffer in bytes
> + * @len: Number of bytes of token data that was retrieved
> + * @result: result of rsi_attestation_token_continue operation
> + */
> +struct arm_cca_token_info {
> + phys_addr_t granule;
> + unsigned long offset;
> + int result;
> +};
> +
> +/**
> + * arm_cca_attestation_continue - Retrieve the attestation token data.
> + *
> + * @param: pointer to the arm_cca_token_info
> + *
> + * Attestation token generation is a long running operation and therefore
> + * the token data may not be retrieved in a single call. Moreover, the
> + * token retrieval operation must be requested on the same CPU on which the
> + * attestation token generation was initialised.
> + * This helper function is therefore scheduled on the same CPU multiple
> + * times until the entire token data is retrieved.
> + */
> +static void arm_cca_attestation_continue(void *param)
> +{
> + unsigned long len;
> + unsigned long size;
> + struct arm_cca_token_info *info;
> +
> + if (!param)
> + return;
This check seems unnecessary and can be dropped.
> +
> + info = (struct arm_cca_token_info *)param;
> +
> + size = RSI_GRANULE_SIZE - info->offset;
> + info->result = rsi_attestation_token_continue(info->granule,
> + info->offset, size, &len);
> + info->offset += len;
> +}
> +
As I suggested in another reply, the return value type of rsi_attestation_token_continue()
needs to be 'unsigned long'. In that case, the type of struct arm_cca_token_info::result
needs to be adjusted either.
> +/**
> + * arm_cca_report_new - Generate a new attestation token.
> + *
> + * @report: pointer to the TSM report context information.
> + * @data: pointer to the context specific data for this module.
> + *
> + * Initialise the attestation token generation using the challenge data
> + * passed in the TSM descriptor. Allocate memory for the attestation token
> + * and schedule calls to retrieve the attestation token on the same CPU
> + * on which the attestation token generation was initialised.
> + *
> + * The challenge data must be at least 32 bytes and no more than 64 bytes. If
> + * less than 64 bytes are provided it will be zero padded to 64 bytes.
> + *
> + * Return:
> + * * %0 - Attestation token generated successfully.
> + * * %-EINVAL - A parameter was not valid.
> + * * %-ENOMEM - Out of memory.
> + * * %-EFAULT - Failed to get IPA for memory page(s).
> + * * A negative status code as returned by smp_call_function_single().
> + */
> +static int arm_cca_report_new(struct tsm_report *report, void *data)
> +{
> + int ret;
> + int cpu;
> + long max_size;
> + unsigned long token_size;
> + struct arm_cca_token_info info;
> + void *buf;
> + u8 *token __free(kvfree) = NULL;
> + struct tsm_desc *desc = &report->desc;
> +
> + if (!report)
> + return -EINVAL;
> +
This check seems unnecessary and can be dropped.
> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
> + return -EINVAL;
> +
> + /*
> + * Get a CPU on which the attestation token generation will be
> + * scheduled and initialise the attestation token generation.
> + */
> + cpu = get_cpu();
> + max_size = rsi_attestation_token_init(desc->inblob, desc->inblob_len);
> + put_cpu();
> +
It seems that put_cpu() is called early, meaning the CPU can go away before
the subsequent call to arm_cca_attestation_continue() ?
> + if (max_size <= 0)
> + return -EINVAL;
> +
> + /* Allocate outblob */
> + token = kvzalloc(max_size, GFP_KERNEL);
> + if (!token)
> + return -ENOMEM;
> +
> + /*
> + * Since the outblob may not be physically contiguous, use a page
> + * to bounce the buffer from RMM.
> + */
> + buf = alloc_pages_exact(RSI_GRANULE_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* Get the PA of the memory page(s) that were allocated. */
> + info.granule = (unsigned long)virt_to_phys(buf);
> +
> + token_size = 0;
This initial assignment can be moved to where the variable is declared.
> + /* Loop until the token is ready or there is an error. */
^^
Maybe it's the personal preference, I personally prefer to avoid the ending
character '.' for the single line of comment.
> + do {
> + /* Retrieve one RSI_GRANULE_SIZE data per loop iteration. */
> + info.offset = 0;
> + do {
> + /*
> + * Schedule a call to retrieve a sub-granule chunk
> + * of data per loop iteration.
> + */
> + ret = smp_call_function_single(cpu,
> + arm_cca_attestation_continue,
> + (void *)&info, true);
> + if (ret != 0) {
> + token_size = 0;
> + goto exit_free_granule_page;
> + }
> +
> + ret = info.result;
> + } while ((ret == RSI_INCOMPLETE) &&
> + (info.offset < RSI_GRANULE_SIZE));
It may be clearer to use 'info.result' here. Besides, unnecessary () exists
in the check.
} while (info.result == RSI_INCOMPLETE &&
info.offset < RSI_GRANULE_SIZE);
Apart from that, we needn't to copy the token over when info.result isn't
RSI_SUCCESS nor RSI_INCOMPLETE.
> +
> + /*
> + * Copy the retrieved token data from the granule
> + * to the token buffer, ensuring that the RMM doesn't
> + * overflow the buffer.
> + */
> + if (WARN_ON(token_size + info.offset > max_size))
> + break;
> + memcpy(&token[token_size], buf, info.offset);
> + token_size += info.offset;
> + } while (ret == RSI_INCOMPLETE);
> +
As above, it may be clearer to use 'info.result' in the check.
} while (info.result == RSI_INCOMPLETE);
> + if (ret != RSI_SUCCESS) {
> + ret = -ENXIO;
> + token_size = 0;
> + goto exit_free_granule_page;
> + }
> +
> + report->outblob = no_free_ptr(token);
> +exit_free_granule_page:
> + report->outblob_len = token_size;
> + free_pages_exact(buf, RSI_GRANULE_SIZE);
> + return ret;
> +}
> +
> +static const struct tsm_ops arm_cca_tsm_ops = {
> + .name = KBUILD_MODNAME,
> + .report_new = arm_cca_report_new,
> +};
> +
> +/**
> + * arm_cca_guest_init - Register with the Trusted Security Module (TSM)
> + * interface.
> + *
> + * Return:
> + * * %0 - Registered successfully with the TSM interface.
> + * * %-ENODEV - The execution context is not an Arm Realm.
> + * * %-EINVAL - A parameter was not valid.
> + * * %-EBUSY - Already registered.
> + */
> +static int __init arm_cca_guest_init(void)
> +{
> + int ret;
> +
> + if (!is_realm_world())
> + return -ENODEV;
> +
> + ret = tsm_register(&arm_cca_tsm_ops, NULL);
> + if (ret < 0)
> + pr_err("Failed to register with TSM.\n");
> +
> + return ret;
> +}
> +module_init(arm_cca_guest_init);
> +
It's probably a bit helpful to print the errno returned from tsm_register().
pr_err("Error %d registering with TSM\n", ret);
The only errno that can be returned from tsm_register() is -EBUSY. So there
is no way for arm_cca_guest_init() to return -EINVAL. The comments need
correction by dropping the description relevant to -EINVAL.
> +/**
> + * arm_cca_guest_exit - unregister with the Trusted Security Module (TSM)
> + * interface.
> + */
> +static void __exit arm_cca_guest_exit(void)
> +{
> + tsm_unregister(&arm_cca_tsm_ops);
> +}
> +module_exit(arm_cca_guest_exit);
> +
> +MODULE_AUTHOR("Sami Mujawar <sami.mujawar@arm.com>");
> +MODULE_DESCRIPTION("Arm CCA Guest TSM Driver.");
> +MODULE_LICENSE("GPL");
The ending character '.' for the module's description may not be needed and can be
dropped.
Thanks,
Gavin
On 08/10/2024 05:12, Gavin Shan wrote:
> On 10/5/24 12:43 AM, Steven Price wrote:
>> From: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Introduce an arm-cca-guest driver that registers with
>> the configfs-tsm module to provide user interfaces for
>> retrieving an attestation token.
>>
>> When a new report is requested the arm-cca-guest driver
>> invokes the appropriate RSI interfaces to query an
>> attestation token.
>>
>> The steps to retrieve an attestation token are as follows:
>> 1. Mount the configfs filesystem if not already mounted
>> mount -t configfs none /sys/kernel/config
>> 2. Generate an attestation token
>> report=/sys/kernel/config/tsm/report/report0
>> mkdir $report
>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>> hexdump -C $report/outblob
>> rmdir $report
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> v3: Minor improvements to comments and adapt to the renaming of
>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>> ---
>> drivers/virt/coco/Kconfig | 2 +
>> drivers/virt/coco/Makefile | 1 +
>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++++++++
>> 5 files changed, 227 insertions(+)
>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>
>> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
>> index d9ff676bf48d..ff869d883d95 100644
>> --- a/drivers/virt/coco/Kconfig
>> +++ b/drivers/virt/coco/Kconfig
>> @@ -14,3 +14,5 @@ source "drivers/virt/coco/pkvm-guest/Kconfig"
>> source "drivers/virt/coco/sev-guest/Kconfig"
>> source "drivers/virt/coco/tdx-guest/Kconfig"
>> +
>> +source "drivers/virt/coco/arm-cca-guest/Kconfig"
>> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
>> index b69c30c1c720..c3d07cfc087e 100644
>> --- a/drivers/virt/coco/Makefile
>> +++ b/drivers/virt/coco/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_EFI_SECRET) += efi_secret/
>> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
>> obj-$(CONFIG_SEV_GUEST) += sev-guest/
>> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
>> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
>> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig
>> b/drivers/virt/coco/arm-cca-guest/Kconfig
>> new file mode 100644
>> index 000000000000..9dd27c3ee215
>> --- /dev/null
>> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
>> @@ -0,0 +1,11 @@
>> +config ARM_CCA_GUEST
>> + tristate "Arm CCA Guest driver"
>> + depends on ARM64
>> + default m
>> + select TSM_REPORTS
>> + help
>> + The driver provides userspace interface to request and
>> + attestation report from the Realm Management Monitor(RMM).
>> +
>> + If you choose 'M' here, this module will be called
>> + arm-cca-guest.
>> diff --git a/drivers/virt/coco/arm-cca-guest/Makefile
>> b/drivers/virt/coco/arm-cca-guest/Makefile
>> new file mode 100644
>> index 000000000000..69eeba08e98a
>> --- /dev/null
>> +++ b/drivers/virt/coco/arm-cca-guest/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest.o
>> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>> b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>> new file mode 100644
>> index 000000000000..e22a565cb425
>> --- /dev/null
>> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>> @@ -0,0 +1,211 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 ARM Ltd.
>> + */
>> +
>> +#include <linux/arm-smccc.h>
>> +#include <linux/cc_platform.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/smp.h>
>> +#include <linux/tsm.h>
>> +#include <linux/types.h>
>> +
>> +#include <asm/rsi.h>
>> +
>> +/**
>> + * struct arm_cca_token_info - a descriptor for the token buffer.
>> + * @granule: PA of the page to which the token will be written
> ^^^^^^^^
>
> s/the page/the granule. They are same thing when we have 4KB page size,
> but there are conceptually different if I'm correct.
Indeed they are different, the granule is always 4KB, the host (and
guest) page size could be bigger (although that's not yet supported).
Here 'granule' does indeed point to a (host) page (because it's returned
from alloc_pages_exact()), but that's just a confusing detail.
I'll update as you suggest.
>> + * @offset: Offset within granule to start of buffer in bytes
>> + * @len: Number of bytes of token data that was retrieved
>> + * @result: result of rsi_attestation_token_continue operation
>> + */
>> +struct arm_cca_token_info {
>> + phys_addr_t granule;
>> + unsigned long offset;
>> + int result;
>> +};
>> +
>> +/**
>> + * arm_cca_attestation_continue - Retrieve the attestation token data.
>> + *
>> + * @param: pointer to the arm_cca_token_info
>> + *
>> + * Attestation token generation is a long running operation and
>> therefore
>> + * the token data may not be retrieved in a single call. Moreover, the
>> + * token retrieval operation must be requested on the same CPU on
>> which the
>> + * attestation token generation was initialised.
>> + * This helper function is therefore scheduled on the same CPU multiple
>> + * times until the entire token data is retrieved.
>> + */
>> +static void arm_cca_attestation_continue(void *param)
>> +{
>> + unsigned long len;
>> + unsigned long size;
>> + struct arm_cca_token_info *info;
>> +
>> + if (!param)
>> + return;
>
> This check seems unnecessary and can be dropped.
Ack
>> +
>> + info = (struct arm_cca_token_info *)param;
>> +
>> + size = RSI_GRANULE_SIZE - info->offset;
>> + info->result = rsi_attestation_token_continue(info->granule,
>> + info->offset, size, &len);
>> + info->offset += len;
>> +}
>> +
>
> As I suggested in another reply, the return value type of
> rsi_attestation_token_continue()
> needs to be 'unsigned long'. In that case, the type of struct
> arm_cca_token_info::result
> needs to be adjusted either.
Ack
>> +/**
>> + * arm_cca_report_new - Generate a new attestation token.
>> + *
>> + * @report: pointer to the TSM report context information.
>> + * @data: pointer to the context specific data for this module.
>> + *
>> + * Initialise the attestation token generation using the challenge data
>> + * passed in the TSM descriptor. Allocate memory for the attestation
>> token
>> + * and schedule calls to retrieve the attestation token on the same CPU
>> + * on which the attestation token generation was initialised.
>> + *
>> + * The challenge data must be at least 32 bytes and no more than 64
>> bytes. If
>> + * less than 64 bytes are provided it will be zero padded to 64 bytes.
>> + *
>> + * Return:
>> + * * %0 - Attestation token generated successfully.
>> + * * %-EINVAL - A parameter was not valid.
>> + * * %-ENOMEM - Out of memory.
>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>> + * * A negative status code as returned by smp_call_function_single().
>> + */
>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>> +{
>> + int ret;
>> + int cpu;
>> + long max_size;
>> + unsigned long token_size;
>> + struct arm_cca_token_info info;
>> + void *buf;
>> + u8 *token __free(kvfree) = NULL;
>> + struct tsm_desc *desc = &report->desc;
>> +
>> + if (!report)
>> + return -EINVAL;
>> +
>
> This check seems unnecessary and can be dropped.
Ack
>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>> + return -EINVAL;
>> +
>> + /*
>> + * Get a CPU on which the attestation token generation will be
>> + * scheduled and initialise the attestation token generation.
>> + */
>> + cpu = get_cpu();
>> + max_size = rsi_attestation_token_init(desc->inblob,
>> desc->inblob_len);
>> + put_cpu();
>> +
>
> It seems that put_cpu() is called early, meaning the CPU can go away before
> the subsequent call to arm_cca_attestation_continue() ?
Indeed, good spot. I'll move it to the end of the function and update
the error paths below.
>> + if (max_size <= 0)
>> + return -EINVAL;
>> +
>> + /* Allocate outblob */
>> + token = kvzalloc(max_size, GFP_KERNEL);
>> + if (!token)
>> + return -ENOMEM;
>> +
>> + /*
>> + * Since the outblob may not be physically contiguous, use a page
>> + * to bounce the buffer from RMM.
>> + */
>> + buf = alloc_pages_exact(RSI_GRANULE_SIZE, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + /* Get the PA of the memory page(s) that were allocated. */
>> + info.granule = (unsigned long)virt_to_phys(buf);
>> +
>> + token_size = 0;
>
> This initial assignment can be moved to where the variable is declared.
Ack
>> + /* Loop until the token is ready or there is an error. */
> ^^
>
> Maybe it's the personal preference, I personally prefer to avoid the ending
> character '.' for the single line of comment.
I agree, my preference is the same. I'll update this.
>> + do {
>> + /* Retrieve one RSI_GRANULE_SIZE data per loop iteration. */
>> + info.offset = 0;
>> + do {
>> + /*
>> + * Schedule a call to retrieve a sub-granule chunk
>> + * of data per loop iteration.
>> + */
>> + ret = smp_call_function_single(cpu,
>> + arm_cca_attestation_continue,
>> + (void *)&info, true);
>> + if (ret != 0) {
>> + token_size = 0;
>> + goto exit_free_granule_page;
>> + }
>> +
>> + ret = info.result;
>> + } while ((ret == RSI_INCOMPLETE) &&
>> + (info.offset < RSI_GRANULE_SIZE));
>
> It may be clearer to use 'info.result' here. Besides, unnecessary () exists
> in the check.
>
> } while (info.result == RSI_INCOMPLETE &&
> info.offset < RSI_GRANULE_SIZE);
Sure
> Apart from that, we needn't to copy the token over when info.result isn't
> RSI_SUCCESS nor RSI_INCOMPLETE.
I'll move the error case up to avoid the copy.
>> +
>> + /*
>> + * Copy the retrieved token data from the granule
>> + * to the token buffer, ensuring that the RMM doesn't
>> + * overflow the buffer.
>> + */
>> + if (WARN_ON(token_size + info.offset > max_size))
>> + break;
>> + memcpy(&token[token_size], buf, info.offset);
>> + token_size += info.offset;
>> + } while (ret == RSI_INCOMPLETE);
>> +
>
> As above, it may be clearer to use 'info.result' in the check.
>
> } while (info.result == RSI_INCOMPLETE);
Ack
>> + if (ret != RSI_SUCCESS) {
>> + ret = -ENXIO;
>> + token_size = 0;
>> + goto exit_free_granule_page;
>> + }
>> +
>> + report->outblob = no_free_ptr(token);
>> +exit_free_granule_page:
>> + report->outblob_len = token_size;
>> + free_pages_exact(buf, RSI_GRANULE_SIZE);
>> + return ret;
>> +}
>> +
>> +static const struct tsm_ops arm_cca_tsm_ops = {
>> + .name = KBUILD_MODNAME,
>> + .report_new = arm_cca_report_new,
>> +};
>> +
>> +/**
>> + * arm_cca_guest_init - Register with the Trusted Security Module (TSM)
>> + * interface.
>> + *
>> + * Return:
>> + * * %0 - Registered successfully with the TSM interface.
>> + * * %-ENODEV - The execution context is not an Arm Realm.
>> + * * %-EINVAL - A parameter was not valid.
>> + * * %-EBUSY - Already registered.
>> + */
>> +static int __init arm_cca_guest_init(void)
>> +{
>> + int ret;
>> +
>> + if (!is_realm_world())
>> + return -ENODEV;
>> +
>> + ret = tsm_register(&arm_cca_tsm_ops, NULL);
>> + if (ret < 0)
>> + pr_err("Failed to register with TSM.\n");
>> +
>> + return ret;
>> +}
>> +module_init(arm_cca_guest_init);
>> +
>
> It's probably a bit helpful to print the errno returned from
> tsm_register().
>
> pr_err("Error %d registering with TSM\n", ret);
>
> The only errno that can be returned from tsm_register() is -EBUSY. So there
> is no way for arm_cca_guest_init() to return -EINVAL. The comments need
> correction by dropping the description relevant to -EINVAL.
Ack
>> +/**
>> + * arm_cca_guest_exit - unregister with the Trusted Security Module
>> (TSM)
>> + * interface.
>> + */
>> +static void __exit arm_cca_guest_exit(void)
>> +{
>> + tsm_unregister(&arm_cca_tsm_ops);
>> +}
>> +module_exit(arm_cca_guest_exit);
>> +
>> +MODULE_AUTHOR("Sami Mujawar <sami.mujawar@arm.com>");
>> +MODULE_DESCRIPTION("Arm CCA Guest TSM Driver.");
>> +MODULE_LICENSE("GPL");
>
> The ending character '.' for the module's description may not be needed
> and can be
> dropped.
Ack.
Thanks for the review!
Steve
On 11/10/2024 15:14, Steven Price wrote:
> On 08/10/2024 05:12, Gavin Shan wrote:
>> On 10/5/24 12:43 AM, Steven Price wrote:
>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>
>>> Introduce an arm-cca-guest driver that registers with
>>> the configfs-tsm module to provide user interfaces for
>>> retrieving an attestation token.
>>>
>>> When a new report is requested the arm-cca-guest driver
>>> invokes the appropriate RSI interfaces to query an
>>> attestation token.
>>>
>>> The steps to retrieve an attestation token are as follows:
>>> 1. Mount the configfs filesystem if not already mounted
>>> mount -t configfs none /sys/kernel/config
>>> 2. Generate an attestation token
>>> report=/sys/kernel/config/tsm/report/report0
>>> mkdir $report
>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>> hexdump -C $report/outblob
>>> rmdir $report
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>> v3: Minor improvements to comments and adapt to the renaming of
>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>> ---
>>> drivers/virt/coco/Kconfig | 2 +
>>> drivers/virt/coco/Makefile | 1 +
>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++++++++
>>> 5 files changed, 227 insertions(+)
>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>>
>>> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
>>> index d9ff676bf48d..ff869d883d95 100644
>>> --- a/drivers/virt/coco/Kconfig
>>> +++ b/drivers/virt/coco/Kconfig
>>> @@ -14,3 +14,5 @@ source "drivers/virt/coco/pkvm-guest/Kconfig"
>>> source "drivers/virt/coco/sev-guest/Kconfig"
>>> source "drivers/virt/coco/tdx-guest/Kconfig"
>>> +
>>> +source "drivers/virt/coco/arm-cca-guest/Kconfig"
>>> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
>>> index b69c30c1c720..c3d07cfc087e 100644
>>> --- a/drivers/virt/coco/Makefile
>>> +++ b/drivers/virt/coco/Makefile
>>> @@ -7,3 +7,4 @@ obj-$(CONFIG_EFI_SECRET) += efi_secret/
>>> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
>>> obj-$(CONFIG_SEV_GUEST) += sev-guest/
>>> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
>>> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
>>> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig
>>> b/drivers/virt/coco/arm-cca-guest/Kconfig
>>> new file mode 100644
>>> index 000000000000..9dd27c3ee215
>>> --- /dev/null
>>> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
>>> @@ -0,0 +1,11 @@
>>> +config ARM_CCA_GUEST
>>> + tristate "Arm CCA Guest driver"
>>> + depends on ARM64
>>> + default m
>>> + select TSM_REPORTS
>>> + help
>>> + The driver provides userspace interface to request and
>>> + attestation report from the Realm Management Monitor(RMM).
>>> +
>>> + If you choose 'M' here, this module will be called
>>> + arm-cca-guest.
>>> diff --git a/drivers/virt/coco/arm-cca-guest/Makefile
>>> b/drivers/virt/coco/arm-cca-guest/Makefile
>>> new file mode 100644
>>> index 000000000000..69eeba08e98a
>>> --- /dev/null
>>> +++ b/drivers/virt/coco/arm-cca-guest/Makefile
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest.o
>>> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>> b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>> new file mode 100644
>>> index 000000000000..e22a565cb425
>>> --- /dev/null
>>> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (C) 2023 ARM Ltd.
>>> + */
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/cc_platform.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/smp.h>
>>> +#include <linux/tsm.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include <asm/rsi.h>
>>> +
>>> +/**
>>> + * struct arm_cca_token_info - a descriptor for the token buffer.
>>> + * @granule: PA of the page to which the token will be written
>> ^^^^^^^^
>>
>> s/the page/the granule. They are same thing when we have 4KB page size,
>> but there are conceptually different if I'm correct.
>
> Indeed they are different, the granule is always 4KB, the host (and
> guest) page size could be bigger (although that's not yet supported).
>
> Here 'granule' does indeed point to a (host) page (because it's returned
> from alloc_pages_exact()), but that's just a confusing detail.
>
> I'll update as you suggest.
>
>>> + * @offset: Offset within granule to start of buffer in bytes
>>> + * @len: Number of bytes of token data that was retrieved
>>> + * @result: result of rsi_attestation_token_continue operation
>>> + */
>>> +struct arm_cca_token_info {
>>> + phys_addr_t granule;
>>> + unsigned long offset;
>>> + int result;
>>> +};
>>> +
>>> +/**
>>> + * arm_cca_attestation_continue - Retrieve the attestation token data.
>>> + *
>>> + * @param: pointer to the arm_cca_token_info
>>> + *
>>> + * Attestation token generation is a long running operation and
>>> therefore
>>> + * the token data may not be retrieved in a single call. Moreover, the
>>> + * token retrieval operation must be requested on the same CPU on
>>> which the
>>> + * attestation token generation was initialised.
>>> + * This helper function is therefore scheduled on the same CPU multiple
>>> + * times until the entire token data is retrieved.
>>> + */
>>> +static void arm_cca_attestation_continue(void *param)
>>> +{
>>> + unsigned long len;
>>> + unsigned long size;
>>> + struct arm_cca_token_info *info;
>>> +
>>> + if (!param)
>>> + return;
>>
>> This check seems unnecessary and can be dropped.
>
> Ack
>
>>> +
>>> + info = (struct arm_cca_token_info *)param;
>>> +
>>> + size = RSI_GRANULE_SIZE - info->offset;
>>> + info->result = rsi_attestation_token_continue(info->granule,
>>> + info->offset, size, &len);
>>> + info->offset += len;
>>> +}
>>> +
>>
>> As I suggested in another reply, the return value type of
>> rsi_attestation_token_continue()
>> needs to be 'unsigned long'. In that case, the type of struct
>> arm_cca_token_info::result
>> needs to be adjusted either.
>
> Ack
>
>>> +/**
>>> + * arm_cca_report_new - Generate a new attestation token.
>>> + *
>>> + * @report: pointer to the TSM report context information.
>>> + * @data: pointer to the context specific data for this module.
>>> + *
>>> + * Initialise the attestation token generation using the challenge data
>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>> token
>>> + * and schedule calls to retrieve the attestation token on the same CPU
>>> + * on which the attestation token generation was initialised.
>>> + *
>>> + * The challenge data must be at least 32 bytes and no more than 64
>>> bytes. If
>>> + * less than 64 bytes are provided it will be zero padded to 64 bytes.
>>> + *
>>> + * Return:
>>> + * * %0 - Attestation token generated successfully.
>>> + * * %-EINVAL - A parameter was not valid.
>>> + * * %-ENOMEM - Out of memory.
>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>> + * * A negative status code as returned by smp_call_function_single().
>>> + */
>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>> +{
>>> + int ret;
>>> + int cpu;
>>> + long max_size;
>>> + unsigned long token_size;
>>> + struct arm_cca_token_info info;
>>> + void *buf;
>>> + u8 *token __free(kvfree) = NULL;
>>> + struct tsm_desc *desc = &report->desc;
>>> +
>>> + if (!report)
>>> + return -EINVAL;
>>> +
>>
>> This check seems unnecessary and can be dropped.
>
> Ack
>
>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * Get a CPU on which the attestation token generation will be
>>> + * scheduled and initialise the attestation token generation.
>>> + */
>>> + cpu = get_cpu();
>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>> desc->inblob_len);
>>> + put_cpu();
>>> +
>>
>> It seems that put_cpu() is called early, meaning the CPU can go away before
>> the subsequent call to arm_cca_attestation_continue() ?
>
> Indeed, good spot. I'll move it to the end of the function and update
> the error paths below.
Actually this was on purpose, not to block the CPU hotplug. The
attestation must be completed on the same CPU.
We can detect the failure from "smp_call" further down and make sure
we can safely complete the operation or restart it.
Suzuki
>
>>> + if (max_size <= 0)
>>> + return -EINVAL;
>>> +
>>> + /* Allocate outblob */
>>> + token = kvzalloc(max_size, GFP_KERNEL);
>>> + if (!token)
>>> + return -ENOMEM;
>>> +
>>> + /*
>>> + * Since the outblob may not be physically contiguous, use a page
>>> + * to bounce the buffer from RMM.
>>> + */
>>> + buf = alloc_pages_exact(RSI_GRANULE_SIZE, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + /* Get the PA of the memory page(s) that were allocated. */
>>> + info.granule = (unsigned long)virt_to_phys(buf);
>>> +
>>> + token_size = 0;
>>
>> This initial assignment can be moved to where the variable is declared.
>
> Ack
>
>>> + /* Loop until the token is ready or there is an error. */
>> ^^
>>
>> Maybe it's the personal preference, I personally prefer to avoid the ending
>> character '.' for the single line of comment.
>
> I agree, my preference is the same. I'll update this.
>
>>> + do {
>>> + /* Retrieve one RSI_GRANULE_SIZE data per loop iteration. */
>>> + info.offset = 0;
>>> + do {
>>> + /*
>>> + * Schedule a call to retrieve a sub-granule chunk
>>> + * of data per loop iteration.
>>> + */
>>> + ret = smp_call_function_single(cpu,
>>> + arm_cca_attestation_continue,
>>> + (void *)&info, true);
>>> + if (ret != 0) {
>>> + token_size = 0;
>>> + goto exit_free_granule_page;
>>> + }
>>> +
>>> + ret = info.result;
>>> + } while ((ret == RSI_INCOMPLETE) &&
>>> + (info.offset < RSI_GRANULE_SIZE));
>>
>> It may be clearer to use 'info.result' here. Besides, unnecessary () exists
>> in the check.
>>
>> } while (info.result == RSI_INCOMPLETE &&
>> info.offset < RSI_GRANULE_SIZE);
>
> Sure
>
>> Apart from that, we needn't to copy the token over when info.result isn't
>> RSI_SUCCESS nor RSI_INCOMPLETE.
>
> I'll move the error case up to avoid the copy.
>
>>> +
>>> + /*
>>> + * Copy the retrieved token data from the granule
>>> + * to the token buffer, ensuring that the RMM doesn't
>>> + * overflow the buffer.
>>> + */
>>> + if (WARN_ON(token_size + info.offset > max_size))
>>> + break;
>>> + memcpy(&token[token_size], buf, info.offset);
>>> + token_size += info.offset;
>>> + } while (ret == RSI_INCOMPLETE);
>>> +
>>
>> As above, it may be clearer to use 'info.result' in the check.
>>
>> } while (info.result == RSI_INCOMPLETE);
>
> Ack
>
>>> + if (ret != RSI_SUCCESS) {
>>> + ret = -ENXIO;
>>> + token_size = 0;
>>> + goto exit_free_granule_page;
>>> + }
>>> +
>>> + report->outblob = no_free_ptr(token);
>>> +exit_free_granule_page:
>>> + report->outblob_len = token_size;
>>> + free_pages_exact(buf, RSI_GRANULE_SIZE);
>>> + return ret;
>>> +}
>>> +
>>> +static const struct tsm_ops arm_cca_tsm_ops = {
>>> + .name = KBUILD_MODNAME,
>>> + .report_new = arm_cca_report_new,
>>> +};
>>> +
>>> +/**
>>> + * arm_cca_guest_init - Register with the Trusted Security Module (TSM)
>>> + * interface.
>>> + *
>>> + * Return:
>>> + * * %0 - Registered successfully with the TSM interface.
>>> + * * %-ENODEV - The execution context is not an Arm Realm.
>>> + * * %-EINVAL - A parameter was not valid.
>>> + * * %-EBUSY - Already registered.
>>> + */
>>> +static int __init arm_cca_guest_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + if (!is_realm_world())
>>> + return -ENODEV;
>>> +
>>> + ret = tsm_register(&arm_cca_tsm_ops, NULL);
>>> + if (ret < 0)
>>> + pr_err("Failed to register with TSM.\n");
>>> +
>>> + return ret;
>>> +}
>>> +module_init(arm_cca_guest_init);
>>> +
>>
>> It's probably a bit helpful to print the errno returned from
>> tsm_register().
>>
>> pr_err("Error %d registering with TSM\n", ret);
>>
>> The only errno that can be returned from tsm_register() is -EBUSY. So there
>> is no way for arm_cca_guest_init() to return -EINVAL. The comments need
>> correction by dropping the description relevant to -EINVAL.
>
> Ack
>
>>> +/**
>>> + * arm_cca_guest_exit - unregister with the Trusted Security Module
>>> (TSM)
>>> + * interface.
>>> + */
>>> +static void __exit arm_cca_guest_exit(void)
>>> +{
>>> + tsm_unregister(&arm_cca_tsm_ops);
>>> +}
>>> +module_exit(arm_cca_guest_exit);
>>> +
>>> +MODULE_AUTHOR("Sami Mujawar <sami.mujawar@arm.com>");
>>> +MODULE_DESCRIPTION("Arm CCA Guest TSM Driver.");
>>> +MODULE_LICENSE("GPL");
>>
>> The ending character '.' for the module's description may not be needed
>> and can be
>> dropped.
>
> Ack.
>
> Thanks for the review!
>
> Steve
>
On 10/12/24 2:22 AM, Suzuki K Poulose wrote:
> On 11/10/2024 15:14, Steven Price wrote:
>> On 08/10/2024 05:12, Gavin Shan wrote:
>>> On 10/5/24 12:43 AM, Steven Price wrote:
>>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>>
>>>> Introduce an arm-cca-guest driver that registers with
>>>> the configfs-tsm module to provide user interfaces for
>>>> retrieving an attestation token.
>>>>
>>>> When a new report is requested the arm-cca-guest driver
>>>> invokes the appropriate RSI interfaces to query an
>>>> attestation token.
>>>>
>>>> The steps to retrieve an attestation token are as follows:
>>>> 1. Mount the configfs filesystem if not already mounted
>>>> mount -t configfs none /sys/kernel/config
>>>> 2. Generate an attestation token
>>>> report=/sys/kernel/config/tsm/report/report0
>>>> mkdir $report
>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>> hexdump -C $report/outblob
>>>> rmdir $report
>>>>
>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>> ---
>>>> v3: Minor improvements to comments and adapt to the renaming of
>>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>>> ---
>>>> drivers/virt/coco/Kconfig | 2 +
>>>> drivers/virt/coco/Makefile | 1 +
>>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++++++++
>>>> 5 files changed, 227 insertions(+)
>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
[...]
>>>> +/**
>>>> + * arm_cca_report_new - Generate a new attestation token.
>>>> + *
>>>> + * @report: pointer to the TSM report context information.
>>>> + * @data: pointer to the context specific data for this module.
>>>> + *
>>>> + * Initialise the attestation token generation using the challenge data
>>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>>> token
>>>> + * and schedule calls to retrieve the attestation token on the same CPU
>>>> + * on which the attestation token generation was initialised.
>>>> + *
>>>> + * The challenge data must be at least 32 bytes and no more than 64
>>>> bytes. If
>>>> + * less than 64 bytes are provided it will be zero padded to 64 bytes.
>>>> + *
>>>> + * Return:
>>>> + * * %0 - Attestation token generated successfully.
>>>> + * * %-EINVAL - A parameter was not valid.
>>>> + * * %-ENOMEM - Out of memory.
>>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>>> + * * A negative status code as returned by smp_call_function_single().
>>>> + */
>>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>>> +{
>>>> + int ret;
>>>> + int cpu;
>>>> + long max_size;
>>>> + unsigned long token_size;
>>>> + struct arm_cca_token_info info;
>>>> + void *buf;
>>>> + u8 *token __free(kvfree) = NULL;
>>>> + struct tsm_desc *desc = &report->desc;
>>>> +
>>>> + if (!report)
>>>> + return -EINVAL;
>>>> +
>>>
>>> This check seems unnecessary and can be dropped.
>>
>> Ack
>>
>>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>>> + return -EINVAL;
>>>> +
>>>> + /*
>>>> + * Get a CPU on which the attestation token generation will be
>>>> + * scheduled and initialise the attestation token generation.
>>>> + */
>>>> + cpu = get_cpu();
>>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>>> desc->inblob_len);
>>>> + put_cpu();
>>>> +
>>>
>>> It seems that put_cpu() is called early, meaning the CPU can go away before
>>> the subsequent call to arm_cca_attestation_continue() ?
>>
>> Indeed, good spot. I'll move it to the end of the function and update
>> the error paths below.
>
> Actually this was on purpose, not to block the CPU hotplug. The
> attestation must be completed on the same CPU.
>
> We can detect the failure from "smp_call" further down and make sure
> we can safely complete the operation or restart it.
>
Yes, It's fine to call put_cpu() early since we're tolerant to error introduced
by CPU unplug. It's a bit confused that rsi_attestation_token_init() is called
on the local CPU while arm_cca_attestation_continue() is called on same CPU
with help of smp_call_function_single(). Does it make sense to unify so that
both will be invoked with the help of smp_call_function_single() ?
int cpu = smp_processor_id();
/*
* The calling and target CPU can be different after the calling process
* is migrated to another different CPU. It's guaranteed the attestatation
* always happen on the target CPU with smp_call_function_single().
*/
ret = smp_call_function_single(cpu, rsi_attestation_token_init_wrapper,
(void *)&info, true);
if (ret) {
...
}
Thanks,
Gavin
On 12/10/2024 07:06, Gavin Shan wrote:
> On 10/12/24 2:22 AM, Suzuki K Poulose wrote:
>> On 11/10/2024 15:14, Steven Price wrote:
>>> On 08/10/2024 05:12, Gavin Shan wrote:
>>>> On 10/5/24 12:43 AM, Steven Price wrote:
>>>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>>>
>>>>> Introduce an arm-cca-guest driver that registers with
>>>>> the configfs-tsm module to provide user interfaces for
>>>>> retrieving an attestation token.
>>>>>
>>>>> When a new report is requested the arm-cca-guest driver
>>>>> invokes the appropriate RSI interfaces to query an
>>>>> attestation token.
>>>>>
>>>>> The steps to retrieve an attestation token are as follows:
>>>>> 1. Mount the configfs filesystem if not already mounted
>>>>> mount -t configfs none /sys/kernel/config
>>>>> 2. Generate an attestation token
>>>>> report=/sys/kernel/config/tsm/report/report0
>>>>> mkdir $report
>>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>> hexdump -C $report/outblob
>>>>> rmdir $report
>>>>>
>>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>>> ---
>>>>> v3: Minor improvements to comments and adapt to the renaming of
>>>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>>>> ---
>>>>> drivers/virt/coco/Kconfig | 2 +
>>>>> drivers/virt/coco/Makefile | 1 +
>>>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211 ++++++++++++
>>>>> ++++++
>>>>> 5 files changed, 227 insertions(+)
>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>
> [...]
>
>>>>> +/**
>>>>> + * arm_cca_report_new - Generate a new attestation token.
>>>>> + *
>>>>> + * @report: pointer to the TSM report context information.
>>>>> + * @data: pointer to the context specific data for this module.
>>>>> + *
>>>>> + * Initialise the attestation token generation using the challenge
>>>>> data
>>>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>>>> token
>>>>> + * and schedule calls to retrieve the attestation token on the
>>>>> same CPU
>>>>> + * on which the attestation token generation was initialised.
>>>>> + *
>>>>> + * The challenge data must be at least 32 bytes and no more than 64
>>>>> bytes. If
>>>>> + * less than 64 bytes are provided it will be zero padded to 64
>>>>> bytes.
>>>>> + *
>>>>> + * Return:
>>>>> + * * %0 - Attestation token generated successfully.
>>>>> + * * %-EINVAL - A parameter was not valid.
>>>>> + * * %-ENOMEM - Out of memory.
>>>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>>>> + * * A negative status code as returned by
>>>>> smp_call_function_single().
>>>>> + */
>>>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>>>> +{
>>>>> + int ret;
>>>>> + int cpu;
>>>>> + long max_size;
>>>>> + unsigned long token_size;
>>>>> + struct arm_cca_token_info info;
>>>>> + void *buf;
>>>>> + u8 *token __free(kvfree) = NULL;
>>>>> + struct tsm_desc *desc = &report->desc;
>>>>> +
>>>>> + if (!report)
>>>>> + return -EINVAL;
>>>>> +
>>>>
>>>> This check seems unnecessary and can be dropped.
>>>
>>> Ack
>>>
>>>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + /*
>>>>> + * Get a CPU on which the attestation token generation will be
>>>>> + * scheduled and initialise the attestation token generation.
>>>>> + */
>>>>> + cpu = get_cpu();
>>>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>>>> desc->inblob_len);
>>>>> + put_cpu();
>>>>> +
>>>>
>>>> It seems that put_cpu() is called early, meaning the CPU can go away
>>>> before
>>>> the subsequent call to arm_cca_attestation_continue() ?
>>>
>>> Indeed, good spot. I'll move it to the end of the function and update
>>> the error paths below.
>>
>> Actually this was on purpose, not to block the CPU hotplug. The
>> attestation must be completed on the same CPU.
>>
>> We can detect the failure from "smp_call" further down and make sure
>> we can safely complete the operation or restart it.
>>
>
> Yes, It's fine to call put_cpu() early since we're tolerant to error
> introduced
> by CPU unplug. It's a bit confused that rsi_attestation_token_init() is
> called
> on the local CPU while arm_cca_attestation_continue() is called on same CPU
> with help of smp_call_function_single(). Does it make sense to unify so
> that
> both will be invoked with the help of smp_call_function_single() ?
>
> int cpu = smp_processor_id();
>
> /*
> * The calling and target CPU can be different after the calling
> process
> * is migrated to another different CPU. It's guaranteed the
> attestatation
> * always happen on the target CPU with smp_call_function_single().
> */
> ret = smp_call_function_single(cpu,
> rsi_attestation_token_init_wrapper,
> (void *)&info, true);
Well, we want to allocate sufficient size buffer (size returned from
token_init()) outside an atomic context (thus not in smp_call_function()).
May be we could make this "allocation" restriction in a comment to
make it clear, why we do it this way.
Suzuki
> if (ret) {
> ...
> }
>
> Thanks,
> Gavin
>
On 14/10/2024 09:56, Suzuki K Poulose wrote:
> On 12/10/2024 07:06, Gavin Shan wrote:
>> On 10/12/24 2:22 AM, Suzuki K Poulose wrote:
>>> On 11/10/2024 15:14, Steven Price wrote:
>>>> On 08/10/2024 05:12, Gavin Shan wrote:
>>>>> On 10/5/24 12:43 AM, Steven Price wrote:
>>>>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>
>>>>>> Introduce an arm-cca-guest driver that registers with
>>>>>> the configfs-tsm module to provide user interfaces for
>>>>>> retrieving an attestation token.
>>>>>>
>>>>>> When a new report is requested the arm-cca-guest driver
>>>>>> invokes the appropriate RSI interfaces to query an
>>>>>> attestation token.
>>>>>>
>>>>>> The steps to retrieve an attestation token are as follows:
>>>>>> 1. Mount the configfs filesystem if not already mounted
>>>>>> mount -t configfs none /sys/kernel/config
>>>>>> 2. Generate an attestation token
>>>>>> report=/sys/kernel/config/tsm/report/report0
>>>>>> mkdir $report
>>>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>>> hexdump -C $report/outblob
>>>>>> rmdir $report
>>>>>>
>>>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>>>> ---
>>>>>> v3: Minor improvements to comments and adapt to the renaming of
>>>>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>>>>> ---
>>>>>> drivers/virt/coco/Kconfig | 2 +
>>>>>> drivers/virt/coco/Makefile | 1 +
>>>>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>>>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>>>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211
>>>>>> ++++++++++++ ++++++
>>>>>> 5 files changed, 227 insertions(+)
>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>
>> [...]
>>
>>>>>> +/**
>>>>>> + * arm_cca_report_new - Generate a new attestation token.
>>>>>> + *
>>>>>> + * @report: pointer to the TSM report context information.
>>>>>> + * @data: pointer to the context specific data for this module.
>>>>>> + *
>>>>>> + * Initialise the attestation token generation using the
>>>>>> challenge data
>>>>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>>>>> token
>>>>>> + * and schedule calls to retrieve the attestation token on the
>>>>>> same CPU
>>>>>> + * on which the attestation token generation was initialised.
>>>>>> + *
>>>>>> + * The challenge data must be at least 32 bytes and no more than 64
>>>>>> bytes. If
>>>>>> + * less than 64 bytes are provided it will be zero padded to 64
>>>>>> bytes.
>>>>>> + *
>>>>>> + * Return:
>>>>>> + * * %0 - Attestation token generated successfully.
>>>>>> + * * %-EINVAL - A parameter was not valid.
>>>>>> + * * %-ENOMEM - Out of memory.
>>>>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>>>>> + * * A negative status code as returned by
>>>>>> smp_call_function_single().
>>>>>> + */
>>>>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + int cpu;
>>>>>> + long max_size;
>>>>>> + unsigned long token_size;
>>>>>> + struct arm_cca_token_info info;
>>>>>> + void *buf;
>>>>>> + u8 *token __free(kvfree) = NULL;
>>>>>> + struct tsm_desc *desc = &report->desc;
>>>>>> +
>>>>>> + if (!report)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>
>>>>> This check seems unnecessary and can be dropped.
>>>>
>>>> Ack
>>>>
>>>>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + /*
>>>>>> + * Get a CPU on which the attestation token generation will be
>>>>>> + * scheduled and initialise the attestation token generation.
>>>>>> + */
>>>>>> + cpu = get_cpu();
>>>>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>>>>> desc->inblob_len);
>>>>>> + put_cpu();
>>>>>> +
>>>>>
>>>>> It seems that put_cpu() is called early, meaning the CPU can go
>>>>> away before
>>>>> the subsequent call to arm_cca_attestation_continue() ?
>>>>
>>>> Indeed, good spot. I'll move it to the end of the function and update
>>>> the error paths below.
>>>
>>> Actually this was on purpose, not to block the CPU hotplug. The
>>> attestation must be completed on the same CPU.
>>>
>>> We can detect the failure from "smp_call" further down and make sure
>>> we can safely complete the operation or restart it.
>>>
>>
>> Yes, It's fine to call put_cpu() early since we're tolerant to error
>> introduced
>> by CPU unplug. It's a bit confused that rsi_attestation_token_init()
>> is called
>> on the local CPU while arm_cca_attestation_continue() is called on
>> same CPU
>> with help of smp_call_function_single(). Does it make sense to unify
>> so that
>> both will be invoked with the help of smp_call_function_single() ?
>>
>> int cpu = smp_processor_id();
>>
>> /*
>> * The calling and target CPU can be different after the calling
>> process
>> * is migrated to another different CPU. It's guaranteed the
>> attestatation
>> * always happen on the target CPU with smp_call_function_single().
>> */
>> ret = smp_call_function_single(cpu,
>> rsi_attestation_token_init_wrapper,
>> (void *)&info, true);
>
> Well, we want to allocate sufficient size buffer (size returned from
> token_init()) outside an atomic context (thus not in smp_call_function()).
>
> May be we could make this "allocation" restriction in a comment to
> make it clear, why we do it this way.
So if I've followed this correctly the get_cpu() route doesn't work
because of the need to allocate outblob. So using
smp_call_function_single() for all calls seems to be the best approach,
along with a comment explaining what's going on. So how about:
/*
* The attestation token 'init' and 'continue' calls must be
* performed on the same CPU. smp_call_function_single() is used
* instead of simply calling get_cpu() because of the need to
* allocate outblob based on the returned value from the 'init'
* call and that cannot be done in an atomic context.
*/
cpu = smp_processor_id();
info.challenge = desc->inblob;
info.challenge_size = desc->inblob_len;
ret = smp_call_function_single(cpu, arm_cca_attestation_init,
&info, true);
if (ret)
return ret;
max_size = info.result;
(with appropriate updates to the 'info' struct and a new
arm_cca_attestation_init() wrapper for rsi_attestation_token_init()).
Steve
On 14/10/2024 15:41, Steven Price wrote:
> On 14/10/2024 09:56, Suzuki K Poulose wrote:
>> On 12/10/2024 07:06, Gavin Shan wrote:
>>> On 10/12/24 2:22 AM, Suzuki K Poulose wrote:
>>>> On 11/10/2024 15:14, Steven Price wrote:
>>>>> On 08/10/2024 05:12, Gavin Shan wrote:
>>>>>> On 10/5/24 12:43 AM, Steven Price wrote:
>>>>>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>>
>>>>>>> Introduce an arm-cca-guest driver that registers with
>>>>>>> the configfs-tsm module to provide user interfaces for
>>>>>>> retrieving an attestation token.
>>>>>>>
>>>>>>> When a new report is requested the arm-cca-guest driver
>>>>>>> invokes the appropriate RSI interfaces to query an
>>>>>>> attestation token.
>>>>>>>
>>>>>>> The steps to retrieve an attestation token are as follows:
>>>>>>> 1. Mount the configfs filesystem if not already mounted
>>>>>>> mount -t configfs none /sys/kernel/config
>>>>>>> 2. Generate an attestation token
>>>>>>> report=/sys/kernel/config/tsm/report/report0
>>>>>>> mkdir $report
>>>>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>>>> hexdump -C $report/outblob
>>>>>>> rmdir $report
>>>>>>>
>>>>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>>>>> ---
>>>>>>> v3: Minor improvements to comments and adapt to the renaming of
>>>>>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>>>>>> ---
>>>>>>> drivers/virt/coco/Kconfig | 2 +
>>>>>>> drivers/virt/coco/Makefile | 1 +
>>>>>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>>>>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>>>>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211
>>>>>>> ++++++++++++ ++++++
>>>>>>> 5 files changed, 227 insertions(+)
>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>>
>>> [...]
>>>
>>>>>>> +/**
>>>>>>> + * arm_cca_report_new - Generate a new attestation token.
>>>>>>> + *
>>>>>>> + * @report: pointer to the TSM report context information.
>>>>>>> + * @data: pointer to the context specific data for this module.
>>>>>>> + *
>>>>>>> + * Initialise the attestation token generation using the
>>>>>>> challenge data
>>>>>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>>>>>> token
>>>>>>> + * and schedule calls to retrieve the attestation token on the
>>>>>>> same CPU
>>>>>>> + * on which the attestation token generation was initialised.
>>>>>>> + *
>>>>>>> + * The challenge data must be at least 32 bytes and no more than 64
>>>>>>> bytes. If
>>>>>>> + * less than 64 bytes are provided it will be zero padded to 64
>>>>>>> bytes.
>>>>>>> + *
>>>>>>> + * Return:
>>>>>>> + * * %0 - Attestation token generated successfully.
>>>>>>> + * * %-EINVAL - A parameter was not valid.
>>>>>>> + * * %-ENOMEM - Out of memory.
>>>>>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>>>>>> + * * A negative status code as returned by
>>>>>>> smp_call_function_single().
>>>>>>> + */
>>>>>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>>>>>> +{
>>>>>>> + int ret;
>>>>>>> + int cpu;
>>>>>>> + long max_size;
>>>>>>> + unsigned long token_size;
>>>>>>> + struct arm_cca_token_info info;
>>>>>>> + void *buf;
>>>>>>> + u8 *token __free(kvfree) = NULL;
>>>>>>> + struct tsm_desc *desc = &report->desc;
>>>>>>> +
>>>>>>> + if (!report)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>
>>>>>> This check seems unnecessary and can be dropped.
>>>>>
>>>>> Ack
>>>>>
>>>>>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * Get a CPU on which the attestation token generation will be
>>>>>>> + * scheduled and initialise the attestation token generation.
>>>>>>> + */
>>>>>>> + cpu = get_cpu();
>>>>>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>>>>>> desc->inblob_len);
>>>>>>> + put_cpu();
>>>>>>> +
>>>>>>
>>>>>> It seems that put_cpu() is called early, meaning the CPU can go
>>>>>> away before
>>>>>> the subsequent call to arm_cca_attestation_continue() ?
>>>>>
>>>>> Indeed, good spot. I'll move it to the end of the function and update
>>>>> the error paths below.
>>>>
>>>> Actually this was on purpose, not to block the CPU hotplug. The
>>>> attestation must be completed on the same CPU.
>>>>
>>>> We can detect the failure from "smp_call" further down and make sure
>>>> we can safely complete the operation or restart it.
>>>>
>>>
>>> Yes, It's fine to call put_cpu() early since we're tolerant to error
>>> introduced
>>> by CPU unplug. It's a bit confused that rsi_attestation_token_init()
>>> is called
>>> on the local CPU while arm_cca_attestation_continue() is called on
>>> same CPU
>>> with help of smp_call_function_single(). Does it make sense to unify
>>> so that
>>> both will be invoked with the help of smp_call_function_single() ?
>>>
>>> int cpu = smp_processor_id();
>>>
>>> /*
>>> * The calling and target CPU can be different after the calling
>>> process
>>> * is migrated to another different CPU. It's guaranteed the
>>> attestatation
>>> * always happen on the target CPU with smp_call_function_single().
>>> */
>>> ret = smp_call_function_single(cpu,
>>> rsi_attestation_token_init_wrapper,
>>> (void *)&info, true);
>>
>> Well, we want to allocate sufficient size buffer (size returned from
>> token_init()) outside an atomic context (thus not in smp_call_function()).
>>
>> May be we could make this "allocation" restriction in a comment to
>> make it clear, why we do it this way.
>
> So if I've followed this correctly the get_cpu() route doesn't work
> because of the need to allocate outblob. So using
> smp_call_function_single() for all calls seems to be the best approach,
> along with a comment explaining what's going on. So how about:
>
> /*
> * The attestation token 'init' and 'continue' calls must be
> * performed on the same CPU. smp_call_function_single() is used
> * instead of simply calling get_cpu() because of the need to
> * allocate outblob based on the returned value from the 'init'
> * call and that cannot be done in an atomic context.
> */
> cpu = smp_processor_id();
>
> info.challenge = desc->inblob;
> info.challenge_size = desc->inblob_len;
>
> ret = smp_call_function_single(cpu, arm_cca_attestation_init,
> &info, true);
> if (ret)
> return ret;
> max_size = info.result;
>
> (with appropriate updates to the 'info' struct and a new
> arm_cca_attestation_init() wrapper for rsi_attestation_token_init()).
That sounds good to me.
Suzuki
On 10/15/24 12:46 AM, Suzuki K Poulose wrote:
> On 14/10/2024 15:41, Steven Price wrote:
>> On 14/10/2024 09:56, Suzuki K Poulose wrote:
>>> On 12/10/2024 07:06, Gavin Shan wrote:
>>>> On 10/12/24 2:22 AM, Suzuki K Poulose wrote:
>>>>> On 11/10/2024 15:14, Steven Price wrote:
>>>>>> On 08/10/2024 05:12, Gavin Shan wrote:
>>>>>>> On 10/5/24 12:43 AM, Steven Price wrote:
>>>>>>>> From: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>>>
>>>>>>>> Introduce an arm-cca-guest driver that registers with
>>>>>>>> the configfs-tsm module to provide user interfaces for
>>>>>>>> retrieving an attestation token.
>>>>>>>>
>>>>>>>> When a new report is requested the arm-cca-guest driver
>>>>>>>> invokes the appropriate RSI interfaces to query an
>>>>>>>> attestation token.
>>>>>>>>
>>>>>>>> The steps to retrieve an attestation token are as follows:
>>>>>>>> 1. Mount the configfs filesystem if not already mounted
>>>>>>>> mount -t configfs none /sys/kernel/config
>>>>>>>> 2. Generate an attestation token
>>>>>>>> report=/sys/kernel/config/tsm/report/report0
>>>>>>>> mkdir $report
>>>>>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>>>>> hexdump -C $report/outblob
>>>>>>>> rmdir $report
>>>>>>>>
>>>>>>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>>>>>> ---
>>>>>>>> v3: Minor improvements to comments and adapt to the renaming of
>>>>>>>> GRANULE_SIZE to RSI_GRANULE_SIZE.
>>>>>>>> ---
>>>>>>>> drivers/virt/coco/Kconfig | 2 +
>>>>>>>> drivers/virt/coco/Makefile | 1 +
>>>>>>>> drivers/virt/coco/arm-cca-guest/Kconfig | 11 +
>>>>>>>> drivers/virt/coco/arm-cca-guest/Makefile | 2 +
>>>>>>>> .../virt/coco/arm-cca-guest/arm-cca-guest.c | 211
>>>>>>>> ++++++++++++ ++++++
>>>>>>>> 5 files changed, 227 insertions(+)
>>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Kconfig
>>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/Makefile
>>>>>>>> create mode 100644 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
>>>>
>>>> [...]
>>>>
>>>>>>>> +/**
>>>>>>>> + * arm_cca_report_new - Generate a new attestation token.
>>>>>>>> + *
>>>>>>>> + * @report: pointer to the TSM report context information.
>>>>>>>> + * @data: pointer to the context specific data for this module.
>>>>>>>> + *
>>>>>>>> + * Initialise the attestation token generation using the
>>>>>>>> challenge data
>>>>>>>> + * passed in the TSM descriptor. Allocate memory for the attestation
>>>>>>>> token
>>>>>>>> + * and schedule calls to retrieve the attestation token on the
>>>>>>>> same CPU
>>>>>>>> + * on which the attestation token generation was initialised.
>>>>>>>> + *
>>>>>>>> + * The challenge data must be at least 32 bytes and no more than 64
>>>>>>>> bytes. If
>>>>>>>> + * less than 64 bytes are provided it will be zero padded to 64
>>>>>>>> bytes.
>>>>>>>> + *
>>>>>>>> + * Return:
>>>>>>>> + * * %0 - Attestation token generated successfully.
>>>>>>>> + * * %-EINVAL - A parameter was not valid.
>>>>>>>> + * * %-ENOMEM - Out of memory.
>>>>>>>> + * * %-EFAULT - Failed to get IPA for memory page(s).
>>>>>>>> + * * A negative status code as returned by
>>>>>>>> smp_call_function_single().
>>>>>>>> + */
>>>>>>>> +static int arm_cca_report_new(struct tsm_report *report, void *data)
>>>>>>>> +{
>>>>>>>> + int ret;
>>>>>>>> + int cpu;
>>>>>>>> + long max_size;
>>>>>>>> + unsigned long token_size;
>>>>>>>> + struct arm_cca_token_info info;
>>>>>>>> + void *buf;
>>>>>>>> + u8 *token __free(kvfree) = NULL;
>>>>>>>> + struct tsm_desc *desc = &report->desc;
>>>>>>>> +
>>>>>>>> + if (!report)
>>>>>>>> + return -EINVAL;
>>>>>>>> +
>>>>>>>
>>>>>>> This check seems unnecessary and can be dropped.
>>>>>>
>>>>>> Ack
>>>>>>
>>>>>>>> + if (desc->inblob_len < 32 || desc->inblob_len > 64)
>>>>>>>> + return -EINVAL;
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * Get a CPU on which the attestation token generation will be
>>>>>>>> + * scheduled and initialise the attestation token generation.
>>>>>>>> + */
>>>>>>>> + cpu = get_cpu();
>>>>>>>> + max_size = rsi_attestation_token_init(desc->inblob,
>>>>>>>> desc->inblob_len);
>>>>>>>> + put_cpu();
>>>>>>>> +
>>>>>>>
>>>>>>> It seems that put_cpu() is called early, meaning the CPU can go
>>>>>>> away before
>>>>>>> the subsequent call to arm_cca_attestation_continue() ?
>>>>>>
>>>>>> Indeed, good spot. I'll move it to the end of the function and update
>>>>>> the error paths below.
>>>>>
>>>>> Actually this was on purpose, not to block the CPU hotplug. The
>>>>> attestation must be completed on the same CPU.
>>>>>
>>>>> We can detect the failure from "smp_call" further down and make sure
>>>>> we can safely complete the operation or restart it.
>>>>>
>>>>
>>>> Yes, It's fine to call put_cpu() early since we're tolerant to error
>>>> introduced
>>>> by CPU unplug. It's a bit confused that rsi_attestation_token_init()
>>>> is called
>>>> on the local CPU while arm_cca_attestation_continue() is called on
>>>> same CPU
>>>> with help of smp_call_function_single(). Does it make sense to unify
>>>> so that
>>>> both will be invoked with the help of smp_call_function_single() ?
>>>>
>>>> int cpu = smp_processor_id();
>>>>
>>>> /*
>>>> * The calling and target CPU can be different after the calling
>>>> process
>>>> * is migrated to another different CPU. It's guaranteed the
>>>> attestatation
>>>> * always happen on the target CPU with smp_call_function_single().
>>>> */
>>>> ret = smp_call_function_single(cpu,
>>>> rsi_attestation_token_init_wrapper,
>>>> (void *)&info, true);
>>>
>>> Well, we want to allocate sufficient size buffer (size returned from
>>> token_init()) outside an atomic context (thus not in smp_call_function()).
>>>
>>> May be we could make this "allocation" restriction in a comment to
>>> make it clear, why we do it this way.
>>
>> So if I've followed this correctly the get_cpu() route doesn't work
>> because of the need to allocate outblob. So using
>> smp_call_function_single() for all calls seems to be the best approach,
>> along with a comment explaining what's going on. So how about:
>>
>> /*
>> * The attestation token 'init' and 'continue' calls must be
>> * performed on the same CPU. smp_call_function_single() is used
>> * instead of simply calling get_cpu() because of the need to
>> * allocate outblob based on the returned value from the 'init'
>> * call and that cannot be done in an atomic context.
>> */
>> cpu = smp_processor_id();
>>
>> info.challenge = desc->inblob;
>> info.challenge_size = desc->inblob_len;
>>
>> ret = smp_call_function_single(cpu, arm_cca_attestation_init,
>> &info, true);
>> if (ret)
>> return ret;
>> max_size = info.result;
>>
>> (with appropriate updates to the 'info' struct and a new
>> arm_cca_attestation_init() wrapper for rsi_attestation_token_init()).
>
> That sounds good to me.
>
+1, it looks good to me as well.
Thanks,
Gavin
Hi Steven,
kernel test robot noticed the following build warnings:
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on linus/master v6.12-rc1 next-20241004]
[cannot apply to efi/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Steven-Price/arm64-rsi-Add-RSI-definitions/20241004-225034
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link: https://lore.kernel.org/r/20241004144307.66199-11-steven.price%40arm.com
patch subject: [PATCH v6 10/11] virt: arm-cca-guest: TSM_REPORT support for realms
config: arm64-randconfig-004-20241005 (https://download.01.org/0day-ci/archive/20241005/202410052337.YQFmvTFu-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241005/202410052337.YQFmvTFu-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410052337.YQFmvTFu-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/virt/coco/arm-cca-guest/arm-cca-guest.c:27: warning: Excess struct member 'len' description in 'arm_cca_token_info'
vim +27 drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
15
16 /**
17 * struct arm_cca_token_info - a descriptor for the token buffer.
18 * @granule: PA of the page to which the token will be written
19 * @offset: Offset within granule to start of buffer in bytes
20 * @len: Number of bytes of token data that was retrieved
21 * @result: result of rsi_attestation_token_continue operation
22 */
23 struct arm_cca_token_info {
24 phys_addr_t granule;
25 unsigned long offset;
26 int result;
> 27 };
28
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2024 Red Hat, Inc.