| 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
| ||
|
Date: Mon, 9 Nov 2015 11:14:39 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Takuya Yoshikawa <yoshikawa_takuya_b1@....ntt.co.jp>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] KVM: x86: MMU: Consolidate WARN_ON/BUG_ON checks for
reverse-mapped sptes
On 06/11/2015 08:25, Takuya Yoshikawa wrote:
> At some call sites of rmap_get_first() and rmap_get_next(), BUG_ON is
> placed right after the call to detect unrelated sptes which should not
> be found in the reverse-mapping list.
>
> Move this check in rmap_get_first/next() so that all call sites, not
> just the users of the for_each_rmap_spte() macro, will be checked the
> same way. In addition, change the BUG_ON to WARN_ON since killing the
> whole host is the last thing that KVM should try.
>
> One thing to keep in mind is that kvm_mmu_unlink_parents() also uses
> rmap_get_first() to handle parent sptes. The change will not break it
> because parent sptes are present, at least until drop_parent_pte()
> actually unlinks them, and not mmio-sptes.
Can you also change kvm_mmu_mark_parents_unsync to use
for_each_rmap_spte instead of pte_list_walk? It is the last use of
pte_list_walk, and it's nice if we have two uses of for_each_rmap_spte
with parent_ptes as the argument.
BTW, on my todo list is to change the rmap items to a struct (with a
single u64 inside) for type safety. Since you are touching this code,
perhaps you can give it a shot?
Paolo
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@....ntt.co.jp>
> ---
> Documentation/virtual/kvm/mmu.txt | 4 ++--
> arch/x86/kvm/mmu.c | 31 ++++++++++++++++++++++---------
> 2 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
> index 3a4d681..daf9c0f 100644
> --- a/Documentation/virtual/kvm/mmu.txt
> +++ b/Documentation/virtual/kvm/mmu.txt
> @@ -203,10 +203,10 @@ Shadow pages contain the following information:
> page cannot be destroyed. See role.invalid.
> parent_ptes:
> The reverse mapping for the pte/ptes pointing at this page's spt. If
> - parent_ptes bit 0 is zero, only one spte points at this pages and
> + parent_ptes bit 0 is zero, only one spte points at this page and
> parent_ptes points at this single spte, otherwise, there exists multiple
> sptes pointing at this page and (parent_ptes & ~0x1) points at a data
> - structure with a list of parent_ptes.
> + structure with a list of parent sptes.
> unsync:
> If true, then the translations in this page may not match the guest's
> translation. This is equivalent to the state of the tlb when a pte is
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index c5e2363..353d752 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1099,17 +1099,28 @@ struct rmap_iterator {
> */
> static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter)
> {
> + u64 *sptep;
> +
> if (!rmap)
> return NULL;
>
> if (!(rmap & 1)) {
> iter->desc = NULL;
> - return (u64 *)rmap;
> + sptep = (u64 *)rmap;
> + goto out;
> }
>
> iter->desc = (struct pte_list_desc *)(rmap & ~1ul);
> iter->pos = 0;
> - return iter->desc->sptes[iter->pos];
> + sptep = iter->desc->sptes[iter->pos];
> +out:
> + /*
> + * Parent sptes found in sp->parent_ptes lists are also checked here
> + * since kvm_mmu_unlink_parents() uses this function. If the condition
> + * needs to be changed for them, make another wrapper function.
> + */
> + WARN_ON(!is_shadow_present_pte(*sptep));
> + return sptep;
> }
>
> /*
> @@ -1119,14 +1130,14 @@ static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter)
> */
> static u64 *rmap_get_next(struct rmap_iterator *iter)
> {
> + u64 *sptep;
> +
> if (iter->desc) {
> if (iter->pos < PTE_LIST_EXT - 1) {
> - u64 *sptep;
> -
> ++iter->pos;
> sptep = iter->desc->sptes[iter->pos];
> if (sptep)
> - return sptep;
> + goto out;
> }
>
> iter->desc = iter->desc->more;
> @@ -1134,17 +1145,20 @@ static u64 *rmap_get_next(struct rmap_iterator *iter)
> if (iter->desc) {
> iter->pos = 0;
> /* desc->sptes[0] cannot be NULL */
> - return iter->desc->sptes[iter->pos];
> + sptep = iter->desc->sptes[iter->pos];
> + goto out;
> }
> }
>
> return NULL;
> +out:
> + WARN_ON(!is_shadow_present_pte(*sptep));
> + return sptep;
> }
>
> #define for_each_rmap_spte(_rmap_, _iter_, _spte_) \
> for (_spte_ = rmap_get_first(*_rmap_, _iter_); \
> - _spte_ && ({BUG_ON(!is_shadow_present_pte(*_spte_)); 1;}); \
> - _spte_ = rmap_get_next(_iter_))
> + _spte_; _spte_ = rmap_get_next(_iter_))
>
> static void drop_spte(struct kvm *kvm, u64 *sptep)
> {
> @@ -1358,7 +1372,6 @@ static bool kvm_zap_rmapp(struct kvm *kvm, unsigned long *rmapp)
> bool flush = false;
>
> while ((sptep = rmap_get_first(*rmapp, &iter))) {
> - BUG_ON(!(*sptep & PT_PRESENT_MASK));
> rmap_printk("%s: spte %p %llx.\n", __func__, sptep, *sptep);
>
> drop_spte(kvm, sptep);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists