Re: kvm: access to invalid memory in mmu_zap_unsync_children

[Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>]

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