Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 15 Aug 2023 16:02:07 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     Sagi Shahar <sagis@google.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
        Sean Christopherson <seanjc@google.com>,
        David Matlack <dmatlack@google.com>,
        Kai Huang <kai.huang@intel.com>,
        Zhi Wang <zhi.wang.linux@gmail.com>, chen.bo@intel.com,
        hang.yuan@intel.com, tina.zhang@intel.com
Subject: Re: [PATCH v15 059/115] KVM: TDX: Create initial guest memory
Message-ID: <20230815230207.GA1436700@private.email.ne.jp>
References: <cover.1690322424.git.isaku.yamahata@intel.com>
 <19a589ab40b01c10c3b9addc5c38f3fe64b15ad0.1690322424.git.isaku.yamahata@intel.com>
 <CAAhR5DG8hHKVjoN+pWKBVivSm8zkBX5NMbKAuWUL2Tkhaj1YRQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAhR5DG8hHKVjoN+pWKBVivSm8zkBX5NMbKAuWUL2Tkhaj1YRQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Tue, Aug 15, 2023 at 11:50:02AM -0700,
Sagi Shahar <sagis@google.com> wrote:

> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index e367351f8d71..32e84c29d35e 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
...
> > @@ -1215,6 +1271,95 @@ void tdx_flush_tlb_current(struct kvm_vcpu *vcpu)
> >         tdx_track(to_kvm_tdx(vcpu->kvm));
> >  }
> >
> > +#define TDX_SEPT_PFERR (PFERR_WRITE_MASK | PFERR_GUEST_ENC_MASK)
> > +
> > +static int tdx_init_mem_region(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > +{
> > +       struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > +       struct kvm_tdx_init_mem_region region;
> > +       struct kvm_vcpu *vcpu;
> > +       struct page *page;
> > +       int idx, ret = 0;
> > +       bool added = false;
> > +
> > +       /* Once TD is finalized, the initial guest memory is fixed. */
> > +       if (is_td_finalized(kvm_tdx))
> > +               return -EINVAL;
> > +
> > +       /* The BSP vCPU must be created before initializing memory regions. */
> > +       if (!atomic_read(&kvm->online_vcpus))
> > +               return -EINVAL;
> > +
> > +       if (cmd->flags & ~KVM_TDX_MEASURE_MEMORY_REGION)
> > +               return -EINVAL;
> > +
> > +       if (copy_from_user(&region, (void __user *)cmd->data, sizeof(region)))
> > +               return -EFAULT;
> > +
> > +       /* Sanity check */
> > +       if (!IS_ALIGNED(region.source_addr, PAGE_SIZE) ||
> > +           !IS_ALIGNED(region.gpa, PAGE_SIZE) ||
> > +           !region.nr_pages ||
> > +           region.gpa + (region.nr_pages << PAGE_SHIFT) <= region.gpa ||
> 
> During an internal security review we noticed that region.nr_pages is
> always checked after it's shifted but when it is used it is not
> shifted. This means that if any of the upper 12 bits are set then we
> will pass the sanity check but the while loop below will run over a
> much larger range than expected.
> 
> A simple fix would be to add the following check to test if any of the
> shifted bits is set:
> +           (region.nr_pages << PAGE_SHIFT) >> PAGE_SHIFT != region.nr_pages ||
> 
> Reported-by: gkirkpatrick@google.com

Thank you for catching it. I ended up with the following check.

  region.nr_pages & GENMASK_ULL(63, 63 - PAGE_SHIFT)

-- 
Isaku Yamahata <isaku.yamahata@gmail.com>