| Message ID | 20201111185337.1237383-1-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: x86/mmu: Fix is_tdp_mmu_check when using PAE | expand |
Ben Gardon wrote: > When PAE is in use, the root_hpa will not have a shadow page assoicated > with it. In this case the kernel will crash with a NULL pointer > dereference. Add checks to ensure is_tdp_mmu_root works as intended even > when using PAE. This seems to work in my amd64 case as well. (https://marc.info/?l=linux-kernel&m=160494962201032&w=2) > Tested: compiles > > Fixes: 02c00b3a2f7e ("kvm: x86/mmu: Allocate and free TDP MMU roots") > Reported-by: Zdenek Kaspar <zkaspar82@gmail.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 27e381c9da6c..13013f4d98ad 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -49,8 +49,18 @@ bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa) > { > struct kvm_mmu_page *sp; > > + if (WARN_ON(!VALID_PAGE(hpa))) > + return false; > + > sp = to_shadow_page(hpa); > > + /* > + * If this VM is being run with PAE, the TDP MMU will not be enabled > + * and the root HPA will not have a shadow page associated with it. > + */ > + if (!sp) > + return false; > + > return sp->tdp_mmu_page && sp->root_count; > } > > -- > 2.29.2.222.g5d2a92d10f8-goog >
On Wed, 11 Nov 2020 10:53:37 -0800 Ben Gardon <bgardon@google.com> wrote: > When PAE is in use, the root_hpa will not have a shadow page > assoicated with it. In this case the kernel will crash with a NULL > pointer dereference. Add checks to ensure is_tdp_mmu_root works as > intended even when using PAE. > > Tested: compiles > > Fixes: 02c00b3a2f7e ("kvm: x86/mmu: Allocate and free TDP MMU roots") > Reported-by: Zdenek Kaspar <zkaspar82@gmail.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 27e381c9da6c..13013f4d98ad 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -49,8 +49,18 @@ bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa) > { > struct kvm_mmu_page *sp; > > + if (WARN_ON(!VALID_PAGE(hpa))) > + return false; > + > sp = to_shadow_page(hpa); > > + /* > + * If this VM is being run with PAE, the TDP MMU will not be > enabled > + * and the root HPA will not have a shadow page associated > with it. > + */ > + if (!sp) > + return false; > + > return sp->tdp_mmu_page && sp->root_count; > } > Fixes is_tdp_mmu_root NULL pointer dereference Tested on: Intel(R) Core(TM)2 CPU 6600 @ 2.40GHz Tested-by: Zdenek Kaspar <zkaspar82@gmail.com>
On 11/11/20 19:53, Ben Gardon wrote: > When PAE is in use, the root_hpa will not have a shadow page assoicated > with it. In this case the kernel will crash with a NULL pointer > dereference. Add checks to ensure is_tdp_mmu_root works as intended even > when using PAE. > > Tested: compiles > > Fixes: 02c00b3a2f7e ("kvm: x86/mmu: Allocate and free TDP MMU roots") > Reported-by: Zdenek Kaspar <zkaspar82@gmail.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 27e381c9da6c..13013f4d98ad 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -49,8 +49,18 @@ bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa) > { > struct kvm_mmu_page *sp; > > + if (WARN_ON(!VALID_PAGE(hpa))) > + return false; > + > sp = to_shadow_page(hpa); > > + /* > + * If this VM is being run with PAE, the TDP MMU will not be enabled > + * and the root HPA will not have a shadow page associated with it. > + */ > + if (!sp) > + return false; > + > return sp->tdp_mmu_page && sp->root_count; > } > > If this was just PAE, it would be easier to test "if (shadow_root_level >= PT64_ROOT_4LEVEL)"---and more correct too, because using the page_private of __pa(vcpu->arch.mmu->pae_root) is a bit untidy; we should only use page_private for pages that we know have a shadow page. In Jamie's case however, it is x86_64 (so kvm_mmu_get_tdp_level(vcpu) == 4 and therefore the "if (shadow_root_level >= PT64_ROOT_4LEVEL)" would be true) but without EPT. In that case we go through vcpu->arch.mmu->root_hpa = __pa(vcpu->arch.mmu->lm_root); but lm_root is allocated with get_zeroed_page and therefore to_shadow_page is NULL. I am thinking of testing simply "if (tdp_enabled)" so that we can see if there are other cases with to_shadow_page(hpa) == NULL and we don't sweep them under the rug. Or test "if (tdp_enabled)" and also WARN if !sp. What do you think? Paolo
On Fri, Nov 13, 2020 at 12:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 11/11/20 19:53, Ben Gardon wrote: > > When PAE is in use, the root_hpa will not have a shadow page assoicated > > with it. In this case the kernel will crash with a NULL pointer > > dereference. Add checks to ensure is_tdp_mmu_root works as intended even > > when using PAE. > > > > Tested: compiles > > > > Fixes: 02c00b3a2f7e ("kvm: x86/mmu: Allocate and free TDP MMU roots") > > Reported-by: Zdenek Kaspar <zkaspar82@gmail.com> > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > arch/x86/kvm/mmu/tdp_mmu.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > > index 27e381c9da6c..13013f4d98ad 100644 > > --- a/arch/x86/kvm/mmu/tdp_mmu.c > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > > @@ -49,8 +49,18 @@ bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa) > > { > > struct kvm_mmu_page *sp; > > > > + if (WARN_ON(!VALID_PAGE(hpa))) > > + return false; > > + > > sp = to_shadow_page(hpa); > > > > + /* > > + * If this VM is being run with PAE, the TDP MMU will not be enabled > > + * and the root HPA will not have a shadow page associated with it. > > + */ > > + if (!sp) > > + return false; > > + > > return sp->tdp_mmu_page && sp->root_count; > > } > > > > > > If this was just PAE, it would be easier to test "if (shadow_root_level > >= PT64_ROOT_4LEVEL)"---and more correct too, because using the > page_private of __pa(vcpu->arch.mmu->pae_root) is a bit untidy; we > should only use page_private for pages that we know have a shadow page. > > In Jamie's case however, it is x86_64 (so kvm_mmu_get_tdp_level(vcpu) == > 4 and therefore the "if (shadow_root_level >= PT64_ROOT_4LEVEL)" would > be true) but without EPT. In that case we go through > > vcpu->arch.mmu->root_hpa = __pa(vcpu->arch.mmu->lm_root); > > but lm_root is allocated with get_zeroed_page and therefore > to_shadow_page is NULL. > > I am thinking of testing simply "if (tdp_enabled)" so that we can see if > there are other cases with to_shadow_page(hpa) == NULL and we don't > sweep them under the rug. Or test "if (tdp_enabled)" and also WARN if > !sp. What do you think? That sounds good to me. I see you already mailed out a modified version of this change, so I'll go review it. Thanks! > > Paolo >
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 27e381c9da6c..13013f4d98ad 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -49,8 +49,18 @@ bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa) { struct kvm_mmu_page *sp; + if (WARN_ON(!VALID_PAGE(hpa))) + return false; + sp = to_shadow_page(hpa); + /* + * If this VM is being run with PAE, the TDP MMU will not be enabled + * and the root HPA will not have a shadow page associated with it. + */ + if (!sp) + return false; + return sp->tdp_mmu_page && sp->root_count; }
When PAE is in use, the root_hpa will not have a shadow page assoicated with it. In this case the kernel will crash with a NULL pointer dereference. Add checks to ensure is_tdp_mmu_root works as intended even when using PAE. Tested: compiles Fixes: 02c00b3a2f7e ("kvm: x86/mmu: Allocate and free TDP MMU roots") Reported-by: Zdenek Kaspar <zkaspar82@gmail.com> Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)