| Message ID | 20210715163159.1480168-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: MMIO guard PV services | expand |
On Thursday 15 Jul 2021 at 17:31:45 (+0100), Marc Zyngier wrote: > Make sure we don't issue CMOs when mapping something that > is not a memory address in the S2 page tables. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 05321f4165e3..a5874ebd0354 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -619,12 +619,16 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > } > > /* Perform CMOs before installation of the guest stage-2 PTE */ > - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) > - mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), > - granule); > - > - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > - mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); > + if (kvm_phys_is_valid(phys)) { > + if (mm_ops->dcache_clean_inval_poc && > + stage2_pte_cacheable(pgt, new)) > + mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, > + mm_ops), > + granule); > + if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > + mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), > + granule); > + } Hrmpf so this makes me realize we have a problem here, not really caused by your patch though. Specifically, calling kvm_pgtable_stage2_set_owner() can lead to overriding valid mappings with invalid mappings, which is effectively an unmap operation. In this case we should issue CMOs when unmapping a cacheable page to ensure it is clean to the PoC, like the kvm_pgtable_stage2_unmap() does. Note that you patch is already an improvement over the current state of things, because calling stage2_pte_cacheable(pgt, new), kvm_pte_follow(new, mm_ops) and friends is bogus when 'new' is invalid ... Thanks, Quentin
On Mon, 19 Jul 2021 18:18:02 +0100, Quentin Perret <qperret@google.com> wrote: > > On Thursday 15 Jul 2021 at 17:31:45 (+0100), Marc Zyngier wrote: > > Make sure we don't issue CMOs when mapping something that > > is not a memory address in the S2 page tables. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index 05321f4165e3..a5874ebd0354 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -619,12 +619,16 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > > } > > > > /* Perform CMOs before installation of the guest stage-2 PTE */ > > - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) > > - mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), > > - granule); > > - > > - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > > - mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); > > + if (kvm_phys_is_valid(phys)) { > > + if (mm_ops->dcache_clean_inval_poc && > > + stage2_pte_cacheable(pgt, new)) > > + mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, > > + mm_ops), > > + granule); > > + if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > > + mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), > > + granule); > > + } > > Hrmpf so this makes me realize we have a problem here, not really caused > by your patch though. > > Specifically, calling kvm_pgtable_stage2_set_owner() can lead to > overriding valid mappings with invalid mappings, which is effectively an > unmap operation. In this case we should issue CMOs when unmapping a > cacheable page to ensure it is clean to the PoC, like the > kvm_pgtable_stage2_unmap() does. You only need that if you to have a non-cacheable mapping to the same page. Otherwise, you will be fine. For pages that are moved from host-EL1 to EL2, we're good (I don't think we ever have a non-cachable mapping at EL2). However, once we start moving pages to guests, we'll need something. > Note that you patch is already an improvement over the current state of > things, because calling stage2_pte_cacheable(pgt, new), > kvm_pte_follow(new, mm_ops) and friends is bogus when 'new' is invalid Yeah, I had it mentally earmarked as a potential stable candidate. We may have to do a bit better in the light of the above though. Thanks, M.
On Thu, Jul 15, 2021 at 05:31:45PM +0100, Marc Zyngier wrote: > Make sure we don't issue CMOs when mapping something that > is not a memory address in the S2 page tables. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 05321f4165e3..a5874ebd0354 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -619,12 +619,16 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > } > > /* Perform CMOs before installation of the guest stage-2 PTE */ > - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) > - mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), > - granule); > - > - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > - mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); > + if (kvm_phys_is_valid(phys)) { > + if (mm_ops->dcache_clean_inval_poc && > + stage2_pte_cacheable(pgt, new)) > + mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, > + mm_ops), > + granule); > + if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > + mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), > + granule); > + } Given that this check corresponds to checking the validity of 'new', I wonder whether we'd be better off pushing the validity checks down into stage2_pte_{cacheable,executable}()? I.e. have stage2_pte_cacheable() return false if !kvm_pte_valid() Will
On Tue, 27 Jul 2021 19:10:45 +0100, Will Deacon <will@kernel.org> wrote: > > On Thu, Jul 15, 2021 at 05:31:45PM +0100, Marc Zyngier wrote: > > Make sure we don't issue CMOs when mapping something that > > is not a memory address in the S2 page tables. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index 05321f4165e3..a5874ebd0354 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -619,12 +619,16 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > > } > > > > /* Perform CMOs before installation of the guest stage-2 PTE */ > > - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) > > - mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), > > - granule); > > - > > - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > > - mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); > > + if (kvm_phys_is_valid(phys)) { > > + if (mm_ops->dcache_clean_inval_poc && > > + stage2_pte_cacheable(pgt, new)) > > + mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, > > + mm_ops), > > + granule); > > + if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) > > + mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), > > + granule); > > + } > > Given that this check corresponds to checking the validity of 'new', I > wonder whether we'd be better off pushing the validity checks down into > stage2_pte_{cacheable,executable}()? > > I.e. have stage2_pte_cacheable() return false if !kvm_pte_valid() That would work just as well. I'll update the patch. Thanks, M.
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index 05321f4165e3..a5874ebd0354 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -619,12 +619,16 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, } /* Perform CMOs before installation of the guest stage-2 PTE */ - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) - mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), - granule); - - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) - mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); + if (kvm_phys_is_valid(phys)) { + if (mm_ops->dcache_clean_inval_poc && + stage2_pte_cacheable(pgt, new)) + mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, + mm_ops), + granule); + if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) + mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), + granule); + } smp_store_release(ptep, new); if (stage2_pte_is_counted(new))
Make sure we don't issue CMOs when mapping something that is not a memory address in the S2 page tables. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)