Re: kvm: access to invalid memory in mmu_zap_unsync_children

From: Takuya Yoshikawa
Date: Mon Jan 18 2016 - 01:26:05 EST


On 2016/01/18 14:00, Xiao Guangrong wrote:
On 01/16/2016 01:06 AM, Paolo Bonzini wrote:
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].

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

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