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

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] [day] [month] [year] [list]

Message-ID: <20230815230207.GA1436700@private.email.ne.jp>
Date:   Tue, 15 Aug 2023 16:02:07 -0700
From:   Isaku Yamahata <isaku.yamahata@...il.com>
To:     Sagi Shahar <sagis@...gle.com>
Cc:     isaku.yamahata@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
        Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
        Sean Christopherson <seanjc@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        Kai Huang <kai.huang@...el.com>,
        Zhi Wang <zhi.wang.linux@...il.com>, chen.bo@...el.com,
        hang.yuan@...el.com, tina.zhang@...el.com
Subject: Re: [PATCH v15 059/115] KVM: TDX: Create initial guest memory

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