linux-kernel - Re: [PATCH v6 10/11] virt: arm-cca-guest: TSM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5999b021-0ae3-4d90-ae29-f18f187fd115@redhat.com>
Date: Sat, 12 Oct 2024 16:06:02 +1000
From: Gavin Shan <gshan@...hat.com>
To: Suzuki K Poulose <suzuki.poulose@....com>,
 Steven Price <steven.price@....com>, kvm@...r.kernel.org,
 kvmarm@...ts.linux.dev
Cc: Sami Mujawar <sami.mujawar@....com>,
 Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>,
 Will Deacon <will@...nel.org>, James Morse <james.morse@....com>,
 Oliver Upton <oliver.upton@...ux.dev>, Zenghui Yu <yuzenghui@...wei.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Joey Gouly <joey.gouly@....com>, Alexandru Elisei
 <alexandru.elisei@....com>, Christoffer Dall <christoffer.dall@....com>,
 Fuad Tabba <tabba@...gle.com>, linux-coco@...ts.linux.dev,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, Alper Gun
 <alpergun@...gle.com>, Dan Williams <dan.j.williams@...el.com>,
 "Aneesh Kumar K . V" <aneesh.kumar@...nel.org>
Subject: Re: [PATCH v6 10/11] virt: arm-cca-guest: TSM_REPORT support for
 realms

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@....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@....com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>>>> Signed-off-by: Steven Price <steven.price@....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