Re: [PATCH v6 10/11] virt: arm-cca-guest: TSM_REPORT support for realms
From: Gavin Shan
Date: Mon Oct 14 2024 - 20:02:06 EST
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@xxxxxxx>
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@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
Signed-off-by: Steven Price <steven.price@xxxxxxx>
---
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