[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/5] x86/32on64: use generic instruction decoding for call gate emulation
>>> On 07.12.16 at 11:55, <andrew.cooper3@xxxxxxxxxx> wrote: > On 06/12/16 13:17, Jan Beulich wrote: >>>>> On 06.12.16 at 12:56, <andrew.cooper3@xxxxxxxxxx> wrote: >>> On 06/12/16 11:12, Jan Beulich wrote: >>>> + return X86EMUL_UNHANDLEABLE; >>>> + >>>> + if ( is_pv_32bit_vcpu(current) ) >>>> + addr = (uint32_t)addr; >>> This should be based on %cs attributes. What about a 64bit PV guest in >>> a compatability segment? >> No - I now realize the conditional is pointless. We only do gate op >> emulation for 32-bit guests. > > Good point. Do we actually ASSERT() this anywhere? It looks like this > properly is only a side effect of emulate_gate_op()'s caller. Why would we assert this anywhere here, when this is just a helper for emulate_gate_op()? I think I've said so elsewhere not too long ago - ASSERT()s are fine, but we shouldn't go overboard with them. >>>> + >>>> + if ( !__addr_ok(addr) || >>>> + (rc = __copy_from_user(p_data, (void *)addr, bytes)) ) >>>> + { >>>> + x86_emul_pagefault(goc->insn_fetch && cpu_has_nx >>>> + ? PFEC_insn_fetch : 0, >>>> + addr + bytes - rc, ctxt); >>> Independently to the point below, the correct predicate for setting >>> PFEC_insn_fetch is ((EFER & EFER_NX) || (CR4 & SMEP)) >> Remember that here we're dealing with PV guests only - those don't >> see any SMEP, and always run with NX enabled as long as we do >> ourselves. > > Conceptually, this is wrong. I agree, but the wrongness is limited to "conceptually", as in practice for PV guests we make certain implications anyway. > For PV guests, we should always base paging decisions on what is really > in EFER and CR4, not on a theoretical guests view of the world. For > pagefaults which we decide isn't relevant to Xen, the propagated fault > will be using the real EFER and CR4. > > In this case (being 32bit only) SMEP is indeed disabled in the context > of the guest, but for 64bit PV guests, Xen's SMEP should be considered, > as it will affect the guests view of hardware. However, ... > >>> However, this is subtly wrong, but I can't think of a sensible way of >>> fixing it (i.e. without doing independent pagewalks). >>> >>> __copy_from_user() does a data fetch, not an instruction fetch. >>> >>> With the advent of PKU, processors support pages which can't be read, >>> but can be executed. Then again, our chances of ever supporting PKU for >>> PV guests is slim, so maybe this point isn't very important. >>> >>> As we actually do a data read, the error code should remain 0. This >>> (getting a data fetch failure for something which should have been an >>> instruction fetch) will be less confusing for the guest than claiming an >>> instruction fetch failure when we actually did a data fetch, as at least >>> the error code will be correct for the access rights in the translation. >> With the above I think this should remain as is. > > ... Why? The problem is still unresolved. > > __copy_from_user() will successfully read a bytestream which was > protected in the pagetables by the NX bit, because it performs a data > read not an instruction fetch. > > In the case of a fault occurred, Xen should report to the guest that it > performed a data read, not an instruction fetch, because that's what it > actually did. > > A guest might rightly expect an instruction fetch fault if it was paying > close attention, but we already currently provide a data read failure, > and this is better than fabricating a fault of what Xen should have done > (but didn't). Okay - I think I need to ask you to stop mixing what this patch does with what may want to be changed, but is orthogonal: We've used copy_from_user() before this patch, so the problem you're pointing out is pre-existing and should not be fixed in this patch. If you think it needs fixing at all, it should be another, follow-up patch. Which leaves us with deciding what error code to use: Of course we could stick with not surfacing PFEC_insn_fetch, but that feels rather wrong as long as we're talking about instruction fetches. But if you insist, I could agree to change it on the basis that the original code also didn't get this right. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |