----- Original Message ----- > From: "Joe Moriarty" <joe.moriarty@xxxxxxxxxx> > To: "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, rkrcmar@xxxxxxxxxx, kvm@xxxxxxxxxxxxxxx, "Xiao Guangrong" > <xiaoguangrong@xxxxxxxxxxx> > Sent: Wednesday, February 14, 2018 7:14:09 PM > Subject: Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem > > On 2/14/2018 12:33 PM, 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?) > > The crux of the problem is the possible return of NULL from > search_memslots() located at line 950 in include/linux/kvm_host.h. This > is called by __gfn_to_memslot(). I would assume this is a case of this > is never going to happen. I say this because the kernel would panic > today with a NULL pointer dereference at line 1254 of arch/x86/kvm/mmu.c > if it did happen. I cannot really rule out that it won't happen. I didn't have in mind that NULL-pointer dereference, but it's always looked racy to me---I've never gotten round to fix it because I wasn't sure about the right thing to do on the rmap_remove path. > I was thinking a BUG_ON() would be more appropriate at the end of > search_memslots() before the "return NULL". No, this is probably too much. The right fix is to pass down the memslot, thus avoiding gfn_to_rmap in favor of __gfn_to_rmap. Paolo
