Hi !

On Thu, Aug 19, 2021 at 4:10 PM Pavel Machek <pavel@...> wrote:

Hi!

CVE-2021-38198: KVM: X86: MMU: Use the correct inherited permissions
to get shadow page

This vulnerability has been introduced since 2.6.20-rc4 so 4.4 affects
this CVE but patch didn't apply to 4.4
(https://lore.kernel.org/stable/162358450944186@kroah.com/). 4.19 also
failed to apply this patch but backport patch has been merged
recently(https://lore.kernel.org/stable/20210812174140.2370680-1-ovidiu.panait@windriver.com/).

I tried to look at this, and it is rather non-trivial. In particular,
I'd not know how to test it. I ended up with this patch, but it is not
even compile-tested.

Best regards,
Pavel

diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
index b653641d4261..ee5bd16a0856 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -152,8 +152,8 @@ Shadow pages contain the following information:
shadow pages) so role.quadrant takes values in the range 0..3. Each
quadrant maps 1GB virtual address space.
role.access:
- Inherited guest access permissions in the form uwx. Note execute
- permission is positive, not negative.
+ Inherited guest access permissions from the parent ptes in the form uwx.
+ Note execute permission is positive, not negative.
role.invalid:
The page is invalid and should not be used. It is a root page that is
currently pinned (by a cpu hardware register pointing to it); once it is
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 7be8a251363e..cebcf7b29b15 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -100,8 +100,8 @@ struct guest_walker {
gpa_t pte_gpa[PT_MAX_FULL_LEVELS];
pt_element_t __user *ptep_user[PT_MAX_FULL_LEVELS];
bool pte_writable[PT_MAX_FULL_LEVELS];
- unsigned pt_access;
- unsigned pte_access;
+ unsigned int pt_access[PT_MAX_FULL_LEVELS];
+ unsigned int pte_access;
gfn_t gfn;
struct x86_exception fault;
};
@@ -354,6 +354,9 @@ retry_walk:
pte_access = pt_access & FNAME(gpte_access)(vcpu, pte);

walker->ptes[walker->level - 1] = pte;
+
+ /* Convert to ACC_*_MASK flags for struct guest_walker. */
+ walker->pt_access[walker->level - 1] = FNAME(gpte_access)(pt_access ^ walk_nx_mask);
} while (!is_last_gpte(mmu, walker->level, pte));

if (unlikely(permission_fault(vcpu, mmu, pte_access, access))) {
@@ -392,10 +395,11 @@ retry_walk:
goto retry_walk;
}

- walker->pt_access = pt_access;
- walker->pte_access = pte_access;
+ walker->pt_access = FNAME(gpte_access)(pt_access ^ walk_nx_mask);
+ walker->pte_access = FNAME(gpte_access)(pte_access ^ walk_nx_mask);
pgprintk("%s: pte %llx pte_access %x pt_access %x\n",
- __func__, (u64)pte, pte_access, pt_access);
+ __func__, (u64)pte, walker->pte_access,
+ walker->pt_access[walker->level - 1]);
return 1;

error:
@@ -555,7 +559,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
{
struct kvm_mmu_page *sp = NULL;
struct kvm_shadow_walk_iterator it;
- unsigned direct_access, access = gw->pt_access;
+ unsigned int direct_access, access;
int top_level, emulate = 0;

direct_access = gw->pte_access;
@@ -586,6 +590,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
sp = NULL;
if (!is_shadow_present_pte(*it.sptep)) {
table_gfn = gw->table_gfn[it.level - 2];
+ access = gw->pt_access[it.level - 2];
sp = kvm_mmu_get_page(vcpu, table_gfn, addr, it.level-1,
false, access, it.sptep);
}

Thank you for the patch. I looked at both original
patch(b1bd5cba3306691c771d558e94baa73e8b0b96b7) and your's.
This patch looks good to me.

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Regards,

--
Masami Ichikawa
Cybertrust Japan Co., Ltd.

Email :masami.ichikawa@...
:masami.ichikawa@...