On 02/25/2018 04:48 AM, Paolo Bonzini wrote:
----- Original Message -----
From: "Xiao Guangrong" <guangrong.xiao@xxxxxxxxx>
To: "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, "Joe Moriarty" <joe.moriarty@xxxxxxxxxx>, rkrcmar@xxxxxxxxxx,
kvm@xxxxxxxxxxxxxxx, "Xiao Guangrong" <xiaoguangrong@xxxxxxxxxxx>
Sent: Saturday, February 24, 2018 4:19:59 AM
Subject: Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
On 02/15/2018 01:33 AM, Paolo Bonzini wrote:
On 14/02/2018 17:55, Joe Moriarty wrote:
Hi Paolo,
Thank you for the review. I wasn't sure if the change I proposed was
correct or not. I will take your suggestion of posting to the mailing
list instead of as a patch the next time I encounter a situation like
this again. In the meantime, I will look at doing your suggestion of
passing kvm_memory_slot down to gfn_to_rmap() and the other functions
you pointed out above for the next version of the patch.
It's not easy, but I can send you a box of beers if you get round to it.
Note that I'm still not sure how the NULL pointer dereference can
happen, and you didn't include more output from your tool, so you might
be wasting your time after all...
Anyway, I would start basically by mapping the paths from try_async_pf's
callers to mmu_set_spte and from there to rmap_add.
On the other hand, in the rmap_remove path, you probably should just
exit immediately if slot is NULL. (Guangrong, do you have any idea why
we don't zap SPTEs in kvm_arch_free_memslot?)
Sorry for the delay replay due to Chinese New Year.
We do zap sptes in kvm_arch_flush_shadow_memslot() if the old memslot is
being
deleted or moved:
kvm_arch_flush_shadow_memslot() -> kvm_page_track_flush_slot()
->kvm_mmu_invalidate_zap_pages_in_memslot()
Right, that is used even if ept=1.
And i do not think the case can happen as we have called gfn_to_pfn()
to check if the gfn is MMIO (see if it exists in memeslots) before
mapping it into rmap. During it, the srcu prevents any memslot
to go away.
It cannot be freed, but it can return NULL the second time. RCU does not
guarantee that no change happen; if you want consistent reads you need to
read each pointer at most once.
That is the reason why invalid memslot comes in.
KVM sets the memslot as invalid first then mmu will see the invalid slot
rather than NULL. A new call of #PF handler will see 'invalid' bit is set
that make the handler directly exit.
