linux-kernel - Re: [PATCH RESEND] KVM: TDX: Use struct_size and simplify tdx_get

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: <4a2a74e01bfd31bc4bd7a672452c2d3d513c33db.camel@intel.com>
Date: Wed, 12 Nov 2025 19:59:13 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "seanjc@...gle.com" <seanjc@...gle.com>, "x86@...nel.org"
	<x86@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "thorsten.blum@...ux.dev"
	<thorsten.blum@...ux.dev>, "bp@...en8.de" <bp@...en8.de>, "mingo@...hat.com"
	<mingo@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"hpa@...or.com" <hpa@...or.com>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"kas@...nel.org" <kas@...nel.org>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND] KVM: TDX: Use struct_size and simplify
 tdx_get_capabilities

On Wed, 2025-11-12 at 18:16 +0100, Thorsten Blum wrote:

kvm x86 logs are suggested to start with a short summary of the patch. Maybe:

Simplify the logic for copying the KVM_TDX_CAPABILITIES struct to userspace.


It looks like you are conducting a treewide pattern matching cleanup?

> > Retrieve the number of user entries with get_user() first and return
> > -E2BIG early if 'user_caps' is too small to fit 'caps'.
> > 
> > Allocate memory for 'caps' only after checking the user buffer's number
> > of entries, thus removing two gotos and the need for premature freeing.
> > 
> > Use struct_size() instead of manually calculating the number of bytes to
> > allocate for 'caps', including the nested flexible array.
> > 
> > Finally, copy 'caps' to user space with a single copy_to_user() call.

In the handling of get_user(nr_user_entries, &user_caps->cpuid.nent), the old
code forced -EFAULT, this patch doesn't. But it leaves the copy_to_user()'s to
still force EFAULT. Why?


Tested-by: Rick Edgecombe <rick.p.edgecombe@...el.com> (really the TDX CI)

> > 
> > Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> > ---
> >  arch/x86/kvm/vmx/tdx.c | 32 ++++++++++++--------------------
> >  1 file changed, 12 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 0a49c863c811..23d638b4a003 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -2282,37 +2282,29 @@ static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
> >  	if (cmd->flags)
> >  		return -EINVAL;
> >  
> > -	caps = kzalloc(sizeof(*caps) +
> > -		       sizeof(struct kvm_cpuid_entry2) * td_conf->num_cpuid_config,
> > -		       GFP_KERNEL);
> > -	if (!caps)
> > -		return -ENOMEM;
> > -
> >  	user_caps = u64_to_user_ptr(cmd->data);
> > -	if (get_user(nr_user_entries, &user_caps->cpuid.nent)) {
> > -		ret = -EFAULT;
> > -		goto out;
> > -	}
> > +	ret = get_user(nr_user_entries, &user_caps->cpuid.nent);
> > +	if (ret)
> > +		return ret;
> >  
> > -	if (nr_user_entries < td_conf->num_cpuid_config) {
> > -		ret = -E2BIG;
> > -		goto out;
> > -	}
> > +	if (nr_user_entries < td_conf->num_cpuid_config)
> > +		return -E2BIG;
> > +
> > +	caps = kzalloc(struct_size(caps, cpuid.entries,
> > +				   td_conf->num_cpuid_config), GFP_KERNEL);
> > +	if (!caps)
> > +		return -ENOMEM;
> >  
> >  	ret = init_kvm_tdx_caps(td_conf, caps);
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (copy_to_user(user_caps, caps, sizeof(*caps))) {
> > +	if (copy_to_user(user_caps, caps, struct_size(caps, cpuid.entries,
> > +						      caps->cpuid.nent))) {
> >  		ret = -EFAULT;
> >  		goto out;
> >  	}
> >  
> > -	if (copy_to_user(user_caps->cpuid.entries, caps->cpuid.entries,
> > -			 caps->cpuid.nent *
> > -			 sizeof(caps->cpuid.entries[0])))
> > -		ret = -EFAULT;
> > -
> >  out:
> >  	/* kfree() accepts NULL. */
> >  	kfree(caps);