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

[Gavin Shan]

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);
    if (ret) {
        ...
    }

    
Thanks,
Gavin