Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Yan Zhao <yan.y.zhao@intel.com>]

On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
...
> +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	struct kvm_tdx_init_vm *init_vm;
> +	struct td_params *td_params = NULL;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(*init_vm) != 256 + sizeof_field(struct kvm_tdx_init_vm, cpuid));
> +	BUILD_BUG_ON(sizeof(struct td_params) != 1024);
> +
> +	if (is_hkid_assigned(kvm_tdx))
> +		return -EINVAL;
> +
> +	if (cmd->flags)
> +		return -EINVAL;
> +
> +	init_vm = kmalloc(sizeof(*init_vm) +
> +			  sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
> +			  GFP_KERNEL);
> +	if (!init_vm)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(init_vm, u64_to_user_ptr(cmd->data), sizeof(*init_vm))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.nent > KVM_MAX_CPUID_ENTRIES) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	if (copy_from_user(init_vm->cpuid.entries,
> +			   u64_to_user_ptr(cmd->data) + sizeof(*init_vm),
> +			   flex_array_size(init_vm, cpuid.entries, init_vm->cpuid.nent))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (memchr_inv(init_vm->reserved, 0, sizeof(init_vm->reserved))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.padding) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	td_params = kzalloc(sizeof(struct td_params), GFP_KERNEL);
> +	if (!td_params) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = setup_tdparams(kvm, td_params, init_vm);
> +	if (ret)
> +		goto out;
> +
> +	ret = __tdx_td_init(kvm, td_params, &cmd->hw_error);
> +	if (ret)
> +		goto out;
> +
> +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> +	kvm_tdx->attributes = td_params->attributes;
> +	kvm_tdx->xfam = td_params->xfam;
> +
> +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> +	else
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> +
Could we introduce a initialized field in struct kvm_tdx and set it true
here? e.g
+       kvm_tdx->initialized = true;

Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
executed successfully? e.g.

@@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
        struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
        struct vcpu_tdx *tdx = to_tdx(vcpu);

+       if (!kvm_tdx->initialized)
+               return -EIO;
+
        /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
        if (!vcpu->arch.apic)
                return -EINVAL;

Allowing vCPU creation only after TD is initialized can prevent unexpected
userspace access to uninitialized TD primitives.
See details in the next comment.

> +out:
> +	/* kfree() accepts NULL. */
> +	kfree(init_vm);
> +	kfree(td_params);
> +
> +	return ret;
> +}
> +
>  int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  {
>  	struct kvm_tdx_cmd tdx_cmd;
> @@ -613,6 +827,9 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  	case KVM_TDX_CAPABILITIES:
>  		r = tdx_get_capabilities(&tdx_cmd);
>  		break;
> +	case KVM_TDX_INIT_VM:
> +		r = tdx_td_init(kvm, &tdx_cmd);
> +		break;
>  	default:
>  		r = -EINVAL;
>  		goto out;


QEMU should invoke VM ioctl KVM_TDX_INIT_VM in tdx_pre_create_vcpu() before
creating vCPUs via VM ioctl KVM_CREATE_VCPU, but KVM should not count on
userspace always doing the right thing.
e.g. running below selftest would produce warning in KVM due to
td_vmcs_write64() error in tdx_load_mmu_pgd().

void verify_td_negative_test(void)
{
        struct kvm_vm *vm;
        struct kvm_vcpu *vcpu;

        vm = td_create();
        vm_enable_cap(vm, KVM_CAP_SPLIT_IRQCHIP, 24);
        vcpu = __vm_vcpu_add(vm, 0);
        vcpu_run(vcpu);
        kvm_vm_free(vm);
}


[ 5600.721996] WARNING: CPU: 116 PID: 7914 at arch/x86/kvm/vmx/tdx.h:237 tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel] 
[ 5600.735999] Modules linked in: kvm_intel kvm idxd i2c_i801 nls_iso8859_1 i2c_smbus i2c_ismt nls_cp437 squashfs hid_generic crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd [last unloaded: kvm]
[ 5600.762904] CPU: 116 PID: 7914 Comm: tdx_vm_tests Not tainted 6.10.0-rc7-upstream+ #278 5e882f76313c2b130a0f7525b7eda06f47d8ea02
[ 5600.779772] Hardware name: Intel Corporation ArcherCity/ArcherCity, BIOS EGSDCRB1.SYS.0101.D29.2303301937 03/30/2023
[ 5600.795940] RIP: 0010:tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel]                  
[ 5600.805013] Code: 00 e8 8f b4 ff ff 48 85 c0 74 52 49 89 c5 48 8b 03 44 0f b6 b0 89 a3 00 00 41 80 fe 01 0f 87 ae 74 00 00 41 83 e6 01 75 1d 90 <0f> 0b 90 48 8b 3b b8 01 01 00 00 be 01 03 00 00 66 89 87 89 a3 00
[ 5600.833286] RSP: 0018:ff3550cf49297c78 EFLAGS: 00010246                       
[ 5600.842233] RAX: ff3550cf4dfd9000 RBX: ff2c5edc10600000 RCX: 0000000000000000 
[ 5600.853400] RDX: 0000000000000000 RSI: ff3550cf49297be8 RDI: 000000000000002b 
[ 5600.864609] RBP: ff3550cf49297c98 R08: 0000000000000000 R09: ffffffffffffffff 
[ 5600.875915] R10: 0000000000000000 R11: 0000000000000000 R12: 000000048d10c000 
[ 5600.887255] R13: c000030000000001 R14: 0000000000000000 R15: 0000000000000000 
[ 5600.898584] FS:  00007f9597799740(0000) GS:ff2c5ee7ad700000(0000) knlGS:0000000000000000
[ 5600.911113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                 
[ 5600.921064] CR2: 00007f959759b8c0 CR3: 000000010b83e005 CR4: 0000000000773ef0 
[ 5600.932675] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 
[ 5600.944319] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400 
[ 5600.955987] PKRU: 55555554                                                    
[ 5600.962665] Call Trace:                                                       
[ 5600.969084]  <TASK>                                                           
[ 5600.975079]  ? show_regs+0x64/0x70                                            
[ 5600.982536]  ? __warn+0x8a/0x100                                              
[ 5600.989840]  ? tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.006321]  ? report_bug+0x1b6/0x220                                         
[ 5601.014351]  ? handle_bug+0x43/0x80                                           
[ 5601.022248]  ? exc_invalid_op+0x18/0x70                                       
[ 5601.030554]  ? asm_exc_invalid_op+0x1b/0x20                                   
[ 5601.039297]  ? tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.056276]  ? tdx_load_mmu_pgd+0x31/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.073270]  vt_load_mmu_pgd+0x57/0x70 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.089991]  kvm_mmu_load+0xa4/0xc0 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.102708]  vcpu_enter_guest+0xbe2/0x1140 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.116042]  ? __this_cpu_preempt_check+0x13/0x20                             
[ 5601.125373]  ? debug_smp_processor_id+0x17/0x20                               
[ 5601.134400]  vcpu_run+0x4d/0x280 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.146657]  ? vcpu_run+0x4d/0x280 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.159108]  kvm_arch_vcpu_ioctl_run+0x224/0x680 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.175943]  kvm_vcpu_ioctl+0x238/0x750 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.188912]  ? __ct_user_exit+0xd1/0x120 
[ 5601.197305]  ? __lock_release.isra.0+0x61/0x160
[ 5601.206432]  ? __ct_user_exit+0xd1/0x120
[ 5601.214791]  __x64_sys_ioctl+0x98/0xd0
[ 5601.222980]  x64_sys_call+0x1222/0x2040
[ 5601.231268]  do_syscall_64+0xc3/0x220
[ 5601.239321]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[ 5601.248913] RIP: 0033:0x7f9597524ded
[ 5601.256781] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 00 00 00
[ 5601.288181] RSP: 002b:00007ffd117315c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 5601.300843] RAX: ffffffffffffffda RBX: 00000000108a32a0 RCX: 00007f9597524ded
[ 5601.313108] RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000005
[ 5601.325411] RBP: 00007ffd11731610 R08: 0000000000422078 R09: 0000000000428e48
[ 5601.337721] R10: 0000000000000001 R11: 0000000000000246 R12: 00000000108a54a0
[ 5601.349965] R13: 0000000000000000 R14: 0000000000434e00 R15: 00007f95977eb000
[ 5601.362131]  </TASK>


> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 268959d0f74f..8912cb6d5bc2 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -16,7 +16,11 @@ struct kvm_tdx {
>  	unsigned long tdr_pa;
>  	unsigned long *tdcs_pa;
>  
> +	u64 attributes;
> +	u64 xfam;
>  	int hkid;
> +
> +	u64 tsc_offset;
>  };

> 
>