| 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
| ||
|
Message-ID: <6d6253a6-c113-44af-856e-118d02e1e409@redhat.com>
Date: Tue, 15 Oct 2024 10:01:39 +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/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@....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);
>>>
>>> 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
Powered by blists - more mailing lists