linux-kernel - Re: [PATCH v19 035/130] KVM: TDX: Add place holder for TDX VM specific mem_enc

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]

Date: Fri, 22 Mar 2024 16:36:58 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	Paolo Bonzini <pbonzini@...hat.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>,
	Sean Christopherson <seanjc@...gle.com>,
	Sagi Shahar <sagis@...gle.com>, "Chen, Bo2" <chen.bo@...el.com>,
	"Yuan, Hang" <hang.yuan@...el.com>,
	"Zhang, Tina" <tina.zhang@...el.com>,
	isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 035/130] KVM: TDX: Add place holder for TDX VM
 specific mem_enc_op ioctl

On Fri, Mar 22, 2024 at 11:10:48AM +1300,
"Huang, Kai" <kai.huang@...el.com> wrote:

> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 45b2c2304491..9ea46d143bef 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -567,6 +567,32 @@ struct kvm_pmu_event_filter {
> >   #define KVM_X86_TDX_VM		2
> >   #define KVM_X86_SNP_VM		3
> > +/* Trust Domain eXtension sub-ioctl() commands. */
> > +enum kvm_tdx_cmd_id {
> > +	KVM_TDX_CAPABILITIES = 0,
> > +
> > +	KVM_TDX_CMD_NR_MAX,
> > +};
> > +
> > +struct kvm_tdx_cmd {
> > +	/* enum kvm_tdx_cmd_id */
> > +	__u32 id;
> > +	/* flags for sub-commend. If sub-command doesn't use this, set zero. */
> > +	__u32 flags;
> > +	/*
> > +	 * data for each sub-command. An immediate or a pointer to the actual
> > +	 * data in process virtual address.  If sub-command doesn't use it,
> > +	 * set zero.
> > +	 */
> > +	__u64 data;
> > +	/*
> > +	 * Auxiliary error code.  The sub-command may return TDX SEAMCALL
> > +	 * status code in addition to -Exxx.
> > +	 * Defined for consistency with struct kvm_sev_cmd.
> > +	 */
> > +	__u64 error;
> 
> If the 'error' is for SEAMCALL error, should we rename it to 'hw_error' or
> 'fw_error' or something similar? I think 'error' is too generic.

Ok, will rename it to hw_error.


> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 5edfb99abb89..07a3f0f75f87 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -55,6 +55,32 @@ struct tdx_info {
> >   /* Info about the TDX module. */
> >   static struct tdx_info *tdx_info;
> > +int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
> > +{
> > +	struct kvm_tdx_cmd tdx_cmd;
> > +	int r;
> > +
> > +	if (copy_from_user(&tdx_cmd, argp, sizeof(struct kvm_tdx_cmd)))
> > +		return -EFAULT;
> 
> Add an empty line.
> 
> > +	if (tdx_cmd.error)
> > +		return -EINVAL;
> 
> Add a comment?
> 
> 	/*
> 	 * Userspace should never set @error, which is used to fill
> 	 * hardware-defined error by the kernel.
> 	 */

Sure.


> > +
> > +	mutex_lock(&kvm->lock);
> > +
> > +	switch (tdx_cmd.id) {
> > +	default:
> > +		r = -EINVAL;
> 
> I am not sure whether you should return -ENOTTY to be consistent with the
> previous vt_mem_enc_ioctl() where a TDX-specific IOCTL is issued for non-TDX
> guest.
>
> Here I think the invalid @id means the sub-command isn't valid.

vt_vcpu_mem_enc_ioctl() checks non-TDX case and returns -ENOTTY.  We know that
the guest is TD.
-- 
Isaku Yamahata <isaku.yamahata@...el.com>