From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CA81FD1812A for ; Mon, 14 Oct 2024 15:36:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=UEre23yMv/1wo0sJ4QkkJ1n5RwH5XvCnz3vNsl3fAbw=; b=W86RfNBItkTfyuDwTz4MnbF1OC nDDyQwc4AMHVmoF6XtgXxorLlMWM16bxJs+rcISBohJc9Z5XuA7Ut18Jxxi81bdtYE8C2HPwYadDC Eg0lZMwZ13ykDQbQLigWtVEMxTYhq9iz2UM5KORlYPzBM3DzNYnrnkz8VM2adZm59LkVgql0nq9Be 0KK7XkR0o7RpUB7kImLfkryrbobek6YYYhCff8k/o6K1Y+E7SEstp6FcHhZBKw4P4fqJTEk/8eQ2b FPw48WDkB81Omh4zsV6daVdEn/JXF8jKzDpXcdZR+Ey5dnbK+/pu9V6YyedlyQFhqX89MIcJqJsq8 w1x/LJjg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t0N6t-00000005gaU-1jlV; Mon, 14 Oct 2024 15:35:51 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t0MG5-00000005Vnu-3xwQ for linux-arm-kernel@lists.infradead.org; Mon, 14 Oct 2024 14:41:19 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6198F1007; Mon, 14 Oct 2024 07:41:44 -0700 (PDT) Received: from [10.57.21.126] (unknown [10.57.21.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BD6853F51B; Mon, 14 Oct 2024 07:41:09 -0700 (PDT) Message-ID: Date: Mon, 14 Oct 2024 15:41:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 10/11] virt: arm-cca-guest: TSM_REPORT support for realms To: Suzuki K Poulose , Gavin Shan , kvm@vger.kernel.org, kvmarm@lists.linux.dev Cc: Sami Mujawar , Catalin Marinas , Marc Zyngier , Will Deacon , James Morse , Oliver Upton , Zenghui Yu , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Joey Gouly , Alexandru Elisei , Christoffer Dall , Fuad Tabba , linux-coco@lists.linux.dev, Ganapatrao Kulkarni , Shanker Donthineni , Alper Gun , Dan Williams , "Aneesh Kumar K . V" References: <20241004144307.66199-1-steven.price@arm.com> <20241004144307.66199-11-steven.price@arm.com> <5a3432d1-6a79-434c-bc93-6317c8c6435c@redhat.com> <6c306817-fbd7-402c-8425-a4523ed43114@arm.com> <7a83461d-40fd-4e61-8833-5dae2abaf82b@arm.com> <5999b021-0ae3-4d90-ae29-f18f187fd115@redhat.com> <11cff100-3406-4608-9993-c29caf3d086d@arm.com> From: Steven Price Content-Language: en-GB In-Reply-To: <11cff100-3406-4608-9993-c29caf3d086d@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241014_074118_237271_1A645373 X-CRM114-Status: GOOD ( 27.31 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 >>>>>> >>>>>> 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 >>>>>> Signed-off-by: Suzuki K Poulose >>>>>> Signed-off-by: Steven Price >>>>>> --- >>>>>> 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()). Steve