Re: [PATCH v2 2/2] KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source

[Sean Christopherson <seanjc@google.com>]

On Wed, Jul 01, 2026, Ackerley Tng wrote:
> sashiko-bot@kernel.org writes:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] The `region.nr_pages << PAGE_SHIFT` calculation can overflow a 64-bit integer, bypassing GPA boundary validation and leading to a host denial of service.
> > --
> >
> > commit a54da29c3e4be3226e4ebf9df2f4182c350df998
> > Author: Sean Christopherson <seanjc@google.com>
> >
> > KVM: TDX: Return EINVAL, not EOPNOTSUPP, for NULL INIT_MEM_REGION source
> >
> > Return EINVAL instead of EOPNOTSUPP if userspace attempts to pass a NULL
> > pointer for the source page of INIT_MEM_REGION, so that KVM's ABI is
> > consistent between TDX and SNP.
> >
> >> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> >> index ffe9d0db58c59..b0ec054732b90 100644
> >> --- a/arch/x86/kvm/vmx/tdx.c
> >> +++ b/arch/x86/kvm/vmx/tdx.c
> >
> > [ ... ]
> >
> >> @@ -3247,8 +3244,8 @@ static int tdx_vcpu_init_mem_region(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *c
> >>  	if (copy_from_user(&region, u64_to_user_ptr(cmd->data), sizeof(region)))
> >>  		return -EFAULT;
> >>
> >> -	if (!PAGE_ALIGNED(region.source_addr) || !PAGE_ALIGNED(region.gpa) ||
> >> -	    !region.nr_pages ||
> >> +	if (!PAGE_ALIGNED(region.source_addr) || !region.source_addr ||
> 
> Since we're changing this, how about separating/grouping conditions so
> that adding validation is clearly different from alignment changes?
> Perhaps like
> 
>     if (!region.source_addr)
> 	return -EINVAL;
> 
>     if (!PAGE_ALIGNED(region.source_addr) || !PAGE_ALIGNED(region.gpa))
> 	return -EINVAL;

I'd rather not do that as part of this patch.  I'm not opposed to refactoring
this code, but I don't want to make the diff harder to read by throwing in
non-trivial changes.

> And this one might need more code if we're fixing the issue Sashiko
> pointed out.
> 
>     if (overflow)
> 	return -EINVAL;
> 
> >> +	    !PAGE_ALIGNED(region.gpa) || !region.nr_pages ||
> 
> Looking at this again, why should region.nr_pages == 0 be -EINVAL? Why
> not do an early exit and return 0?

Because nr_pages == 0 is nonsensical.  And as a general rule, don't return success
for uAPI unless it's necessary, because it's much, much harder to extend
functionality if a path can succeed than if a path is guaranteed to fail.

E.g. very hypothetically, if we want to repurpose '0' to mean "delete this page"
or something, then returning -EINVAL now provides a more viable route to supporting
such an operation.