Skip to contentSkip to conversation

kvm: access to invalid memory in mmu_zap_unsync_children

DISCUSSION Sasha Levin 4 participants 4 messages Jan 15, 2016 – Jan 18, 2016

lore
Share & tools
lore

Sasha Levin

author
#0 lore raw

Hi all,

While fuzzing with syzkaller on the latest -next kernel running on a KVM tools guest, I've hit the following invalid memory access:

[  547.956284] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17
[  547.956940] index 3 is out of range for type 'kvm_mmu_page *[3]'
[  547.957567] CPU: 0 PID: 21577 Comm: syz-executor Tainted: G      D         4.4.0-next-20160114-sasha-00021-gf1273d1-dirty #2798
[  547.958739]  1ffff1001819be5c 000000002fa0e55b ffff8800c0cdf360 ffffffff83433c4e
[  547.972448]  0000000041b58ab3 ffffffff8f960c38 ffffffff83433b86 ffff8800c0cdf328
[  547.973277]  0000000000000001 000000002fa0e55b ffffffff8feb8440 ffff8800c0cdf3f0
[  547.974102] Call Trace:
[  547.974424] dump_stack (lib/dump_stack.c:52)
[  547.975774] ubsan_epilogue (lib/ubsan.c:165)
[  547.976408] __ubsan_handle_out_of_bounds (lib/ubsan.c:382)
[  547.980877] mmu_zap_unsync_children (arch/x86/kvm/mmu.c:2011 arch/x86/kvm/mmu.c:2272)
[  548.014193] kvm_mmu_prepare_zap_page (arch/x86/kvm/mmu.c:2294)
[  548.018228] kvm_mmu_invalidate_zap_all_pages (arch/x86/kvm/mmu.c:4737 arch/x86/kvm/mmu.c:4776)
[  548.018994] kvm_arch_flush_shadow_all (arch/x86/kvm/x86.c:8050)
[  548.019696] kvm_mmu_notifier_release (include/linux/rcupdate.h:495 include/linux/srcu.h:237 arch/x86/kvm/../../../virt/kvm/kvm_main.c:447)
[  548.021657] __mmu_notifier_release (mm/mmu_notifier.c:66 (discriminator 19))
[  548.024574] exit_mmap (include/linux/mmu_notifier.h:235 mm/mmap.c:2819)
[  548.058887] mmput (kernel/fork.c:707)
[  548.059572] do_exit (./arch/x86/include/asm/bitops.h:311 include/linux/thread_info.h:92 kernel/exit.c:437 kernel/exit.c:735)
[  548.061733] do_group_exit (include/linux/sched.h:813 kernel/exit.c:861)
[  548.062400] get_signal (kernel/signal.c:2327)
[  548.063063] do_signal (arch/x86/kernel/signal.c:781)
[  548.071394] exit_to_usermode_loop (arch/x86/entry/common.c:247)
[  548.072066] syscall_return_slowpath (arch/x86/entry/common.c:282 arch/x86/entry/common.c:344)
[  548.072824] int_ret_from_sys_call (arch/x86/entry/entry_64.S:282)

Thanks,
Sashagl

#1 lore raw
Quoted 15 lines · Sasha Levin

Hi all,

While fuzzing with syzkaller on the latest -next kernel running on a KVM tools guest, I've hit the following invalid memory access:

[  547.956284] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17
[  547.956940] index 3 is out of range for type 'kvm_mmu_page *[3]'
[  547.957567] CPU: 0 PID: 21577 Comm: syz-executor Tainted: G      D         4.4.0-next-20160114-sasha-00021-gf1273d1-dirty #2798
[  547.958739]  1ffff1001819be5c 000000002fa0e55b ffff8800c0cdf360 ffffffff83433c4e
[  547.972448]  0000000041b58ab3 ffffffff8f960c38 ffffffff83433b86 ffff8800c0cdf328
Quoted 8 lines
[  547.973277]  0000000000000001 000000002fa0e55b ffffffff8feb8440 ffff8800c0cdf3f0
[  547.974102] Call Trace:
[  547.974424] dump_stack (lib/dump_stack.c:52)
[  547.975774] ubsan_epilogue (lib/ubsan.c:165)
[  547.976408] __ubsan_handle_out_of_bounds (lib/ubsan.c:382)
[  547.980877] mmu_zap_unsync_children (arch/x86/kvm/mmu.c:2011 arch/x86/kvm/mmu.c:2272)

Marcelo/Takuya/Xiao,

do you know what's the point in the assignment in kvm_mmu_pages_init?

It seems to me that it should be

	parents->parent[0] = NULL;

since the only user of the ->parent[] array, mmu_pages_clear_parents, walks the array up from parents->parent[0].

Any other opinions?

Paolo

#2 lore raw
Quoted 29 lines · Paolo Bonzini
Quoted 15 lines · Sasha Levin

Hi all,

While fuzzing with syzkaller on the latest -next kernel running on a KVM tools guest, I've hit the following invalid memory access:

[  547.956284] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17
[  547.956940] index 3 is out of range for type 'kvm_mmu_page *[3]'
[  547.957567] CPU: 0 PID: 21577 Comm: syz-executor Tainted: G      D         4.4.0-next-20160114-sasha-00021-gf1273d1-dirty #2798
[  547.958739]  1ffff1001819be5c 000000002fa0e55b ffff8800c0cdf360 ffffffff83433c4e
[  547.972448]  0000000041b58ab3 ffffffff8f960c38 ffffffff83433b86 ffff8800c0cdf328
Quoted 8 lines
[  547.973277]  0000000000000001 000000002fa0e55b ffffffff8feb8440 ffff8800c0cdf3f0
[  547.974102] Call Trace:
[  547.974424] dump_stack (lib/dump_stack.c:52)
[  547.975774] ubsan_epilogue (lib/ubsan.c:165)
[  547.976408] __ubsan_handle_out_of_bounds (lib/ubsan.c:382)
[  547.980877] mmu_zap_unsync_children (arch/x86/kvm/mmu.c:2011 arch/x86/kvm/mmu.c:2272)

Marcelo/Takuya/Xiao,

Quoted 9 lines

do you know what's the point in the assignment in kvm_mmu_pages_init?

It seems to me that it should be

	parents->parent[0] = NULL;

since the only user of the ->parent[] array, mmu_pages_clear_parents, walks the array up from parents->parent[0].

Yes, it is bugly and it surprised me that it was not triggered in nested env.

Quoted 1 line

Any other opinions?

The idea we use the array as [PT64_ROOT_LEVEL-1] is because we never take the last level (level = 1) into account.

I think this diff can fix this, but it has not tested yet.

+#define INVALID_INDEX (-1)
+
static int mmu_unsync_walk(struct kvm_mmu_page *sp,
struct kvm_mmu_pages *pvec)
{
if (!sp->unsync_children)
return 0;

- mmu_pages_add(pvec, sp, 0);
+ /*
+ * do not count the index in the parent of the sp we're
+ * walking start from.
+ */
+ mmu_pages_add(pvec, sp, INVALID_INDEX);
return __mmu_unsync_walk(sp, pvec);
}

@@ -1980,8 +1986,11 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec,
return n;
}

- parents->parent[sp->role.level-2] = sp;
- parents->idx[sp->role.level-1] = pvec->page[n].idx;
+ parents->parent[sp->role.level - 2] = sp;
+
+ /* skip setting idex of the sp we start from. */
+ if (pvec->page[n].idx != INVALID_INDEX)
+ parents->idx[sp->role.level - 1] = pvec->page[n].idx;
}

return n;
@@ -1999,6 +2008,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents)
if (!sp)
return;

+ WARN_ON(idx != INVALID_INDEX);
clear_unsync_child_bit(sp, idx);
level++;
} while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children);
@@ -2008,7 +2018,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent,
struct mmu_page_path *parents,
struct kvm_mmu_pages *pvec)
{
- parents->parent[parent->role.level-1] = NULL;
+ parents->parent[parent->role.level - 2] = NULL;
pvec->nr = 0;
}

Quoted 3 lines

Paolo

#3 lore raw
Quoted 34 lines · Xiao Guangrong
Quoted 33 lines · Paolo Bonzini
Quoted 22 lines · Sasha Levin

Hi all,

While fuzzing with syzkaller on the latest -next kernel running on a KVM tools
guest, I've hit the following invalid memory access:

[  547.956284] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17
[  547.956940] index 3 is out of range for type 'kvm_mmu_page *[3]'
[  547.957567] CPU: 0 PID: 21577 Comm: syz-executor Tainted: G

D 4.4.0-next-20160114-sasha-00021-gf1273d1-dirty #2798

[  547.958739]  1ffff1001819be5c 000000002fa0e55b ffff8800c0cdf360

ffffffff83433c4e

[  547.972448]  0000000041b58ab3 ffffffff8f960c38 ffffffff83433b86

ffff8800c0cdf328

[  547.973277]  0000000000000001 000000002fa0e55b ffffffff8feb8440

ffff8800c0cdf3f0

Quoted 7 lines
[  547.974102] Call Trace:
[  547.974424] dump_stack (lib/dump_stack.c:52)
[  547.975774] ubsan_epilogue (lib/ubsan.c:165)
[  547.976408] __ubsan_handle_out_of_bounds (lib/ubsan.c:382)
[  547.980877] mmu_zap_unsync_children (arch/x86/kvm/mmu.c:2011

arch/x86/kvm/mmu.c:2272)

Marcelo/Takuya/Xiao,

Quoted 8 lines
Quoted 8 lines

do you know what's the point in the assignment in kvm_mmu_pages_init?

It seems to me that it should be

    parents->parent[0] = NULL;

since the only user of the ->parent[] array, mmu_pages_clear_parents, walks the array up from parents->parent[0].

Triggering "index 3 is out of range for type 'kvm_mmu_page *[3]'" in the first kvm_mmu_pages_init() call in mmu_zap_unsync_children() means the parent passed in to mmu_zap_unsync_children() has its ->role.level set to PT64_ROOT_LEVEL.

Is this the problem being reported?

Maybe, I'm just confused by the incorrect line-numbers and it's possible that the problem was triggered in the while loop.

Quoted 2 lines

Yes, it is bugly and it surprised me that it was not triggered in nested env.

Yes, the code is not changed much from the following commit:
KVM: MMU: use page array in unsync walk
commit 60c8aec6e2c9923492dabbd6b67e34692bd26c20

What, including my recent cleanups, could change the situation to make this happen? I'm not sure. It's almost seven years.

Quoted 4 lines
Quoted 1 line

Any other opinions?

The idea we use the array as [PT64_ROOT_LEVEL-1] is because we never take the last level (level = 1) into account.

Yes, this is reasonable.

Quoted 1 line

I think this diff can fix this, but it has not tested yet.

I'm still reading the code but not sure what changed the situation.

Takuya

Quoted 50 lines
(quoted diff) +12 -3
+#define INVALID_INDEX    (-1)
+
  static int mmu_unsync_walk(struct kvm_mmu_page *sp,
                 struct kvm_mmu_pages *pvec)
  {
      if (!sp->unsync_children)
          return 0;
 
-    mmu_pages_add(pvec, sp, 0);
+    /*
+     * do not count the index in the parent of the sp we're
+     * walking start from.
+     */
+    mmu_pages_add(pvec, sp, INVALID_INDEX);
      return __mmu_unsync_walk(sp, pvec);
  }

@@ -1980,8 +1986,11 @@ static int mmu_pages_next(struct kvm_mmu_pages
 *pvec,
              return n;
          }
 
-        parents->parent[sp->role.level-2] = sp;
-        parents->idx[sp->role.level-1] = pvec->page[n].idx;
+        parents->parent[sp->role.level - 2] = sp;
+
+        /* skip setting idex of the sp we start from. */
+        if (pvec->page[n].idx != INVALID_INDEX)
+            parents->idx[sp->role.level - 1] = pvec->page[n].idx;
      }
 

      return n;
(quoted diff) +1 -0
@@ -1999,6 +2008,7 @@ static void mmu_pages_clear_parents(struct
 mmu_page_path *parents)
          if (!sp)
              return;
 
+        WARN_ON(idx != INVALID_INDEX);
          clear_unsync_child_bit(sp, idx);
          level++;

      } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children);
(quoted diff) +1 -1
@@ -2008,7 +2018,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page
 *parent,
                     struct mmu_page_path *parents,
                     struct kvm_mmu_pages *pvec)
  {
-    parents->parent[parent->role.level-1] = NULL;
+    parents->parent[parent->role.level - 2] = NULL;
      pvec->nr = 0;
  }