linux-kernel - Re: [Part1 PATCH v6 16/17] X86/KVM: Decrypt shared per-cpu variables when SEV is active

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: <de38a1ba-98f6-e7c8-427d-f717fa126db5@amd.com>
Date:   Mon, 16 Oct 2017 20:43:15 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [Part1 PATCH v6 16/17] X86/KVM: Decrypt shared per-cpu variables
 when SEV is active



On 10/16/17 5:24 PM, Borislav Petkov wrote:
...

>>  
>> +static inline void __set_percpu_decrypted(void *ptr, unsigned long size)
>> +{
>> +	early_set_memory_decrypted(slow_virt_to_phys(ptr), size);
>> +}
> Ok, so this looks like useless conversion:
>
> you pass in a virtual address, it gets converted to a physical address
> with slow_virt_to_phys() and then in early_set_memory_enc_dec() gets
> converted to a virtual address again.
>
> Why? Why not pass the virtual address directly?


Actually, I worked to enable the kvmclock support before the
kvm-stealtime, eoi and apf_reason. The kvmclock uses memblock_alloc() to
allocate the shared memory and since the memblock_alloc() returns the
physical address hence I used the same input type as a argument to the
early_set_memory_decrypted(). If you want me to change the input to
accept the virtual address then I have no issue doing so. But the
changes need to propagated to kvmclock (i.e PATCH 17/17) to use __va().
Please let me know if you want me to pass the virtual address.


>> +/*
>> + * Iterate through all possible CPUs and map the memory region pointed
>> + * by apf_reason, steal_time and kvm_apic_eoi as decrypted at once.
>> + *
>> + * Note: we iterate through all possible CPUs to ensure that CPUs
>> + * hotplugged will have their per-cpu variable already mapped as
>> + * decrypted.
>> + */
>> +static void __init sev_map_percpu_data(void)
>> +{
>> +	int cpu;
>> +
>> +	if (!sev_active())
>> +		return;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		__set_percpu_decrypted(&per_cpu(apf_reason, cpu), sizeof(apf_reason));
>> +		__set_percpu_decrypted(&per_cpu(steal_time, cpu), sizeof(steal_time));
>> +		__set_percpu_decrypted(&per_cpu(kvm_apic_eoi, cpu), sizeof(kvm_apic_eoi));
>> +	}
>> +}
>> +
>>  #ifdef CONFIG_SMP
>>  static void __init kvm_smp_prepare_boot_cpu(void)
>>  {
>> +	sev_map_percpu_data();
>>  	kvm_guest_cpu_init();
>>  	native_smp_prepare_boot_cpu();
>>  	kvm_spinlock_init();
>> @@ -496,6 +524,7 @@ void __init kvm_guest_init(void)
>>  				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
>>  		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
>>  #else
>> +	sev_map_percpu_data();
>>  	kvm_guest_cpu_init();
>>  #endif
> Why isn't it enough to call
>
> 	sev_map_percpu_data()
>
> at the end of kvm_guest_init() only but you have to call it in
> kvm_smp_prepare_boot_cpu() too? I mean, once you map those things
> decrypted, there's no need to do them again...
>

IIRC, we tried clearing C bit in kvm_guest_init() but since the
kvm_guest_init() is called before setup_per_cpu_areas() hence
per_cpu_ptr(var, cpu_id) was not able to get another processors copy of
the variable.