[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 11:23, Jan Beulich wrote:
>>>> 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.

Sorry if it was not clear, but my objection was to this patch changing
the error code (as this makes Xen's behaviour even more wrong than it
was before), not the continued use of copy_from_user() (which I don't
see a resonable alternative to at the moment).

If you don't mind, please keep this patch maintaining the previous
behaviour, and we can address the pagefault behaviour separately.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.