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

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

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
>