From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751619AbcARG0C (ORCPT ); Mon, 18 Jan 2016 01:26:02 -0500 Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:35453 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750862AbcARGZ7 (ORCPT ); Mon, 18 Jan 2016 01:25:59 -0500 Subject: Re: kvm: access to invalid memory in mmu_zap_unsync_children References: <56992438.6090804@oracle.com> <56992719.9060004@redhat.com> <569C718A.9090802@linux.intel.com> From: Takuya Yoshikawa Message-ID: <569C856B.3060507@lab.ntt.co.jp> Date: Mon, 18 Jan 2016 15:25:47 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <569C718A.9090802@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit To: Xiao Guangrong , Paolo Bonzini , Sasha Levin , Gleb Natapov Cc: LKML , Dmitry Vyukov , syzkaller , Marcelo Tosatti X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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; > }