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