kvm: access to invalid memory in mmu_zap_unsync_children

25 views
Skip to first unread message

Sasha Levin

unread,
Jan 15, 2016, 11:54:24 AM1/15/16
to Gleb Natapov, Paolo Bonzini, LKML, Dmitry Vyukov, syzkaller
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

Paolo Bonzini

unread,
Jan 15, 2016, 12:06:54 PM1/15/16
to Sasha Levin, Gleb Natapov, LKML, Dmitry Vyukov, syzkaller, Marcelo Tosatti, Takuya Yoshikawa, Xiao Guangrong


On 15/01/2016 17:54, Sasha Levin wrote:
> 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)

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

Xiao Guangrong

unread,
Jan 18, 2016, 12:08:11 AM1/18/16
to Paolo Bonzini, Sasha Levin, Gleb Natapov, LKML, Dmitry Vyukov, syzkaller, Marcelo Tosatti, Takuya Yoshikawa
Yes, it is bugly and it surprised me that it was not triggered in nested env.

> 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;
}


>
> Paolo
>

Takuya Yoshikawa

unread,
Jan 18, 2016, 1:25:45 AM1/18/16
to Xiao Guangrong, Paolo Bonzini, Sasha Levin, Gleb Natapov, LKML, Dmitry Vyukov, syzkaller, Marcelo Tosatti
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.

> 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.

>> 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.

> 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
Reply all
Reply to author
Forward
0 new messages