Hi,
On Mon, May 16, 2011, Marcelo Tosatti wrote about "Re: [PATCH 12/30] nVMX: Implement VMPTRLD":
> > + if (vmx->nested.current_vmptr != vmcs12_addr) {
> > + struct vmcs12 *new_vmcs12;
> > + struct page *page;
> > + page = nested_get_page(vcpu, vmcs12_addr);
> > + if (page == NULL) {
> > + nested_vmx_failInvalid(vcpu);
>
> This can access a NULL current_vmcs12 pointer, no?
I'm afraid I didn't understand where this specific code you can access a
NULL current_vmcs12 pointer...
Looking at the rest of this function, the code, for examples, that frees the
previous current_vmcs12_page, first makes sure that
vmx->nested.current_vmptr != -1ull. current_vmptr, current_vmcs12 and
current_vmcs12_page are all set together (when everything was succesful)
and we never release the old page before we test the new one, so we can be
sure that whenever current_vmptr != -1, we have a valid current_vmcs12 as well.
But maybe I'm missing something?
> Apparently other
> code paths are vulnerable to the same issue (as in allowed to execute
> before vmtprld maps guest VMCS). Perhaps a BUG_ON on get_vmcs12 could be
> helpful.
The call to get_vmcs12() should typically be in a if(is_guest_mode(vcpu))
(or in places we know this to be true), and to enter L2, we should have
verified already that we have a working vmcs12. This is why I thought it
is unnecessary to add any assertions to the trivial inline function
get_vmcs12().
But now that I think about it, there does appear to be a problem in
nested_vmx_run(): This is where we should have verified that there is a
current VMCS - i.e., that VMPTRLD was previously used! And it seems I forgot
testing this... :( I'll need to add such a test - not as a BUG_ON but as
a real test that causes the VMLAUNCH instruction to fail (I have to look at
the spec to see exactly how) if VMPTRLD hadn't been previously done.
--
Nadav Har'El | Monday, May 16 2011, 13 Iyyar 5771
nyh@xxxxxxxxxxxxxxxxxxx |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |I have a great signature, but it won't
http://nadav.harel.org.il |fit at the end of this message -- Fermat
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
