Re: [PATCH v6 10/11] virt: arm-cca-guest: TSM_REPORT support for realms

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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