Re: [PATCH v15 059/115] KVM: TDX: Create initial guest memory

[Isaku Yamahata <isaku.yamahata@xxxxxxxxx>]

On Tue, Aug 15, 2023 at 11:50:02AM -0700,
Sagi Shahar <sagis@xxxxxxxxxx> 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@xxxxxxxxxx

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@xxxxxxxxx>