linux-kernel - Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

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: <ed2453e8-a95f-9b17-b3e1-a9355e8e04fe@amd.com>
Date:   Wed, 5 Apr 2023 08:24:14 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     nikunj@....com, linux-kernel@...r.kernel.org, x86@...nel.org
Cc:     bp@...en8.de, dionnaglaze@...gle.com, pgonda@...gle.com,
        seanjc@...gle.com, pbonzini@...hat.com, michael.roth@....com,
        ketanch@...k.ac.in
Subject: Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

On 4/5/23 02:37, Nikunj A. Dadhania wrote:
> On 4/4/2023 3:11 AM, Tom Lendacky wrote:
>> On 3/26/23 09:46, Nikunj A Dadhania wrote:
>>> Add support for Secure TSC in SNP enabled guests. Secure TSC
>>> allows guest to securely use RDTSC/RDTSCP instructions as the
>>> parameters being used cannot be changed by hypervisor once the
>>> guest is launched.
>>>
>>> During the boot-up of the secondary cpus, SecureTSC enabled
>>> guests need to query TSC info from Security processor (PSP).
>>
>> s/Security processor (PSP)/AMD Secure Processor/
>>
>>> This communication channel is encrypted between the security
>>
>> here as well.
>>
>>> processor and the guest, hypervisor is just the conduit to
>>
>> s/hypervisor/the hypervisor/
>>
> 
> Sure, will change commit message
> 
>>> deliver the guest messages to the security processor. Each
>>> message is protected with an AEAD (AES-256 GCM). Use minimal
>>> GCM library to encrypt/decrypt SNP Guest messages to communicate
>>> with the PSP.
>>>
>>> Moreover, the hypervisor should not be intercepting RDTSC/RDTSCP
>>> when Secure TSC is enabled. A #VC exception will be generated if
>>> the RDTSC/RDTSCP instructions are being intercepted. If this should
>>> occur and Secure TSC is enabled, terminate guest execution.
>>
>> This seems like a separate patch.
> 
> Sure.
> 
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
>>> ---
> 
>>> diff --git a/arch/x86/include/asm/sev-guest.h b/arch/x86/include/asm/sev-guest.h
>>> index 834cdae302ad..d5ed041ce06b 100644
>>> --- a/arch/x86/include/asm/sev-guest.h
>>> +++ b/arch/x86/include/asm/sev-guest.h
> 
>>> +#define SNP_TSC_INFO_REQ_SZ 128
>>> +    /* Must be zero filled */
>>> +    u8 rsvd[SNP_TSC_INFO_REQ_SZ];
>>> +} __packed;
>>> +
>>> +struct snp_tsc_info_resp {
>>> +    /* Status of TSC_INFO message */
>>> +    u32 status;
>>> +    u32 rsvd1;
>>> +    u64 tsc_scale;
>>> +    u64 tsc_offset;
>>> +    u64 tsc_factor;
>>
>> This should be a u32 ...
>>
> 
> Right, will correct.
>   
>>> +    u8 rsvd2[96];
>>
>> Which then makes this 100.
> 
> Yes.
> 
>>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>>> index 3750e545d688..280aaa1e6aad 100644
>>> --- a/arch/x86/kernel/sev.c
>>> +++ b/arch/x86/kernel/sev.c
> 
>>> +bool __init snp_secure_tsc_prepare(void)
>>> +{
>>> +    platform_data = kzalloc(sizeof(*platform_data), GFP_KERNEL);
>>> +    if (!platform_data)
>>> +        return false;
>>> +
>>> +    /* Initialize the PSP channel to send snp messages */
>>> +    if (snp_setup_psp_messaging(platform_data))
>>> +        sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>>> +
>>> +    if (cc_platform_has(CC_ATTR_GUEST_SECURE_TSC)) {
>>
>> Should this be checked before allocating memory and calling snp_setup_psp_messaging()?
> 
> No, as we need guest messaging to work without Secure TSC as well.

My take is that the sev-guest driver should establish the key it is going 
to use at the time the driver is loaded. snp_setup_psp_messaging() should 
be called by whatever is going to use guest messaging.

Having a generic routine that is accessed by both the kernel and the 
driver should be the goal. Maybe it is best to only have the vmpck_id be 
part of any structure and then the appropriate key and sequence number are 
used based on that id when the request is made.

The sev_guest_platform_data struct can just hold context information (it 
doesn't need the secrets_gpa any more since everything is now in sev.c 
which knows what that value is) for whatever is using guest messaging.

> 
>   
>> Also, I notice here you use the cc_platform_has() function but in previous
>> patches you check sev_status directly.
> 
> For sev-shared.c, cc_platform_has() is not available when compiling boot/compressed.
> 
> I will change the below call site to cc_platfrom_has() after testing.
> 
> arch/x86/kernel/sev.c:  if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) {
> 
>> And you don't implement support for
>> CC_ATTR_GUEST_SECURE_TSC until the last patch instead of now.
> 
> This is to make sure that SECURE_TSC is enabled only after the last patch.
> As cc_platform_has() is being used at multiple places to enable the feature.

But you terminate long before that in snp_check_features() since you don't 
update SNP_FEATURES_PRESENT with SECURE_TSC until the last patch, right?

Thanks,
Tom

> 
>> You can't get here until SNP_FEATURES_PRESENT is updated.
>>
>>> +        if (snp_get_tsc_info())
>>> +            sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>>> +
>>> +        pr_info("SecureTSC enabled\n");
>>> +    }
>>
>> Blank line.
> 
> Sure
> 
> Regards
> Nikunj
>