Skip to contentSkip to conversation

KVM: prevent read from desc->shadow_ptes[-1] in rmap_desc_remove_entry()

PATCH In review Roel Kluin 2 participants 2 messages Aug 29, 2009

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 KVM: prevent read from desc->shadow_ptes[-1] in rmap_desc_remove_entry()

prevent read from desc->shadow_ptes[-1]

Signed-off-by Roel Kluin <[email protected]>

---
If in rmap_remove() (bottom) we do:

while (desc) {
        for (i = 0; i < RMAP_EXT && desc->shadow_ptes[i]; ++i)
                if (desc->shadow_ptes[i] == spte) {
                        rmap_desc_remove_entry(rmapp,
                                               desc, i,
                                               prev_desc);
                        return;
                }
        prev_desc = desc;
        desc = desc->more;
}

If in the first iteration esc->shadow_ptes[0] == spte, then we call rmap_desc_remove_entry() with i == 0, and then we read in the last iteration from desc->shadow_ptes[-1].

I found this by code analysis.

arch/x86/kvm/mmu.c +1 -1
index 0ef5bb2..e1b2e46 100644
@@ -541,7 +541,7 @@ static void rmap_desc_remove_entry(unsigned long *rmapp,
 {
 	int j;
 
-	for (j = RMAP_EXT - 1; !desc->shadow_ptes[j] && j > i; --j)
+	for (j = RMAP_EXT - 1; j > i && !desc->shadow_ptes[j]; --j)
 		;
 	desc->shadow_ptes[i] = desc->shadow_ptes[j];
 	desc->shadow_ptes[j] = NULL;
#1 lore raw
Quoted 30 lines · Roel Kluin

prevent read from desc->shadow_ptes[-1]

Signed-off-by Roel Kluin<[email protected]>

---
If in rmap_remove() (bottom) we do:

while (desc) {
         for (i = 0; i<  RMAP_EXT&&  desc->shadow_ptes[i]; ++i)
                 if (desc->shadow_ptes[i] == spte) {
                         rmap_desc_remove_entry(rmapp,
                                                desc, i,
                                                prev_desc);
                         return;
                 }
         prev_desc = desc;
         desc = desc->more;
}

If in the first iteration esc->shadow_ptes[0] == spte, then we call rmap_desc_remove_entry() with i == 0, and then we read in the last iteration from desc->shadow_ptes[-1].

I found this by code analysis.

arch/x86/kvm/mmu.c +0 -0
index 0ef5bb2..e1b2e46 100644

arch/x86/kvm/mmu.c static void rmap_desc_remove_entry(unsigned long *rmapp, · line 547

Quoted 8 lines
arch/x86/kvm/mmu.c +1 -1
@@ -541,7 +541,7 @@ static void rmap_desc_remove_entry(unsigned long *rmapp,
  {
  	int j;
 
-	for (j = RMAP_EXT - 1; !desc->shadow_ptes[j]&&  j>  i; --j)
+	for (j = RMAP_EXT - 1; j>  i&&  !desc->shadow_ptes[j]; --j)
  		;
  	desc->shadow_ptes[i] = desc->shadow_ptes[j];
  	desc->shadow_ptes[j] = NULL;

There is never a case when j < 0. The loop finds the highest j that has a non-NULL shadow_pte[j], and that is guaranteed to exist.

I think the j > i test is redundant. Not sure though.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.