Re: [Xen-devel] [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/11/2018 10:53, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 13 November 2018 10:47
>> To: Jan Beulich <JBeulich@xxxxxxxx>; xen-devel <xen-
>> devel@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
>> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
>> p2m_ioreq_server pages
>>
>> On 13/11/18 10:13, Jan Beulich wrote:
>>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
>>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
>>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
>>> p2m_ioreq_server special case. That change widened a pre-existing issue
>>> though: Other writes to such pages also need to be failed (or forced
>>> through emulation), in particular hypercall buffer writes.
>>>
>>> Reported-by: ??? <???@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>>>          if ( res != HVMTRANS_okay )
>>>              return res;
>>>
>>> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
>> While this does address the issue, I'm concerned about hardcoding the
>> behaviour here.
>>
>> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
>> as-yet unspecified per-ioreq-client meaning.
>>
>> We either want to rename p2m_ioreq_server to something which indicates
>> its "allow-reads/emulate writes" behaviour, or design a way for the
>> ioreq client to specify the behaviour it wants.
>>
> The comment in the public header is:
>
> /*                                                                           
>  * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id> 
>                                                                            
>  *                                      to specific memory type <type>       
>  *                                      for specific accesses <flags>        
>  *                                                                           
>  * For now, flags only accept the value of XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,  
>  * which means only write operations are to be forwarded to an ioreq server. 
>  * Support for the emulation of read operations can be added when an ioreq   
>  * server has such requirement in future.                                    
>  */
>
> ...so the write-intercept-only behaviour is baked in. Whilst I agree it would 
> be nice not to proliferate this, I don't think it needs addressing in the 
> short term.

Lovely :(

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel