linux-kernel - Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send

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: <3dd645f2-476a-d0d5-c8c1-c87307f2d756@amd.com>
Date: Mon, 10 Mar 2025 08:27:37 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Stefano Garzarella <sgarzare@...hat.com>, Borislav Petkov <bp@...en8.de>
Cc: Jarkko Sakkinen <jarkko@...nel.org>, Thomas Gleixner
 <tglx@...utronix.de>, Claudio Carvalho <cclaudio@...ux.ibm.com>,
 Peter Huewe <peterhuewe@....de>, x86@...nel.org,
 Dov Murik <dovmurik@...ux.ibm.com>, linux-coco@...ts.linux.dev,
 Dionna Glaze <dionnaglaze@...gle.com>,
 James Bottomley <James.Bottomley@...senpartnership.com>,
 Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
 Jason Gunthorpe <jgg@...pe.ca>, linux-integrity@...r.kernel.org,
 linux-kernel@...r.kernel.org, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command
 functions

On 3/10/25 07:46, Stefano Garzarella wrote:
> On Mon, Mar 10, 2025 at 12:30:06PM +0100, Borislav Petkov wrote:
>> On Fri, Feb 28, 2025 at 06:07:16PM +0100, Stefano Garzarella wrote:
>>> +bool snp_svsm_vtpm_probe(void)
>>> +{
>>> +    struct svsm_call call = {};
>>> +    u64 send_cmd_mask = 0;
>>> +    u64 platform_cmds;
>>> +    u64 features;
>>> +    int ret;
>>> +
>>> +    /* The vTPM device is available only if we have a SVSM */
>>
>> s/if we have a SVSM/if an SVSM is present/
>>
>>> +    if (!snp_vmpl)
>>> +        return false;
>>> +
>>> +    call.caa = svsm_get_caa();
>>> +    call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>> +
>>> +    ret = svsm_perform_call_protocol(&call);
>>> +
>>
>>
>> ^ Superfluous newline.
>>
>>> +    if (ret != SVSM_SUCCESS)
>>> +        return false;
>>> +
>>> +    features = call.rdx_out;
>>> +    platform_cmds = call.rcx_out;
>>> +
>>> +    /* No feature supported, it should be zero */
>>> +    if (features)
>>> +        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>>> +            features);
>>
>> So
>>
>>     return false;
>>
>> here?
> 
> In v1 we had that, but Tom Lendacky suggested to remove it:
> https://lore.kernel.org/linux-integrity/4valfkw7wtx3fpdv2qbymzggcu7mp4mhkd65j5q7zncs2dzorc@jjjevuwfchgl/
> 
> IIUC the features are supposed to be additive, so Tom's point was to
> avoid that in the future SVSM will supports new features and this driver
> stops working, when it could, just without using the new features.
> 
> I added a warning just to be aware of new features, but I can remove it.

I don't think anything needs to be checked or printed. If you want to do
anything, just issue a pr_info() with the features value (and maybe the
platform_cmds value, too). Issuing a pr_warn() here would be like
issuing a pr_warn() for a new CPUID value that the current kernel
doesn't know about.

Thanks,
Tom

> 
>>
>>> +
>>> +    /* TPM_SEND_COMMAND - platform command 8 */
>>> +    send_cmd_mask = 1 << 8;
>>
>>     BIT_ULL(8);
>>
>>> +
>>> +    return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>> +
>>> +int snp_svsm_vtpm_send_command(u8 *buffer)
>>> +{
>>> +    struct svsm_call call = {};
>>> +
>>> +    call.caa = svsm_get_caa();
>>> +    call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
>>> +    call.rcx = __pa(buffer);
>>> +
>>> +    return svsm_perform_call_protocol(&call);
>>> +}
>>
>> In any case, you can zap all those local vars, use comments instead
>> and slim
>> down the function, diff ontop:
> 
> Thanks for the diff, I'll apply it except, for now, the return in the
> feature check which is still not clear to me (I think I get Tom's point,
> but I would like confirmation from both of you).
> 
> Thanks,
> Stefano
> 
>>
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 3902af4b1385..6d7e97c1f567 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -2631,12 +2631,9 @@ static int snp_issue_guest_request(struct
>> snp_guest_req *req, struct snp_req_dat
>> bool snp_svsm_vtpm_probe(void)
>> {
>>     struct svsm_call call = {};
>> -    u64 send_cmd_mask = 0;
>> -    u64 platform_cmds;
>> -    u64 features;
>>     int ret;
>>
>> -    /* The vTPM device is available only if we have a SVSM */
>> +    /* The vTPM device is available only if a SVSM is present */
>>     if (!snp_vmpl)
>>         return false;
>>
>> @@ -2644,22 +2641,17 @@ bool snp_svsm_vtpm_probe(void)
>>     call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>
>>     ret = svsm_perform_call_protocol(&call);
>> -
>>     if (ret != SVSM_SUCCESS)
>>         return false;
>>
>> -    features = call.rdx_out;
>> -    platform_cmds = call.rcx_out;
>> -
>>     /* No feature supported, it should be zero */
>> -    if (features)
>> -        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> -            features);
>> -
>> -    /* TPM_SEND_COMMAND - platform command 8 */
>> -    send_cmd_mask = 1 << 8;
>> +    if (call.rdx_out) {
>> +        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> call.rdx_out);
>> +        return false;
>> +    }
>>
>> -    return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>> +    /* Check platform commands is TPM_SEND_COMMAND - platform command
>> 8 */
>> +    return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
>> }
>> EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>
>>
>> -- 
>> Regards/Gruss,
>>    Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
>