Re: [Xen-devel] [PATCH RFC v2 1/4] x86/mm: Shadow and p2m changes for PV mem_access

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.07.14 at 01:34, <aravindp@xxxxxxxxx> wrote:
>>> +
>>> +            /* If the access is against the permissions, then send to 
>>> mem_event
>>*/
>>> +            switch ( p2ma )
>>> +            {
>>> +            case p2m_access_r:
>>> +                violation = access_w || access_x;
>>> +                break;
>>> +            case p2m_access_rx:
>>> +            case p2m_access_rx2rw:
>>> +                violation = access_w;
>>> +                break;
>>> +            case p2m_access_rw:
>>> +                violation = access_x;
>>> +                break;
>>> +            case p2m_access_rwx:
>>> +            default:
>>> +                break;
>>> +            }
>>> +
>>> +            /*
>>> +             * Do not police writes to guest memory from the Xen 
>>> hypervisor.
>>> +             * This keeps PV mem_access on par with HVM. Turn off CR0.WP
>>here to
>>> +             * allow the write to go through if the guest has marked the 
>>> page as
>>> +             * writable. Turn it back on in the guest access functions
>>> +             * __copy_to_user / __put_user_size() after the write is 
>>> completed.
>>> +             */
>>> +            if ( violation && access_w &&
>>> +                 regs->eip >= XEN_VIRT_START && regs->eip <= XEN_VIRT_END )
>>
>>Definitely < instead of <= on the right side. But - is this safe, the more
>>that this again doesn't appear to be sitting in a guest kind specific block?
>>I'd at least expect this to be qualified by a regs->cs and/or
>>guest_mode() check.
> 
> I will add the guest kind check. Should I do it for the whole block i.e add 
> is_pv_domain() in addition to mem_event_check_ring()?

Since mem_event_check_ring() isn't PV specific, you'd need to go
through and amend any such checks with a PV one where needed.
And yes, doing it once for the whole code block seems like the
right thing to do here.

> That will also address 
> next comment below. I will add the guest_mode() in addition to the above 
> check for policing Xen writes to guest memory.
> 
>>> +            {
>>> +                unsigned long cr0 = read_cr0();
>>> +
>>> +                violation = 0;
>>> +                if ( cr0 & X86_CR0_WP &&
>>> +                     guest_l1e_get_flags(gw.l1e) & _PAGE_RW )
>>> +                {
>>> +                    cr0 &= ~X86_CR0_WP;
>>> +                    write_cr0(cr0);
>>> +                    v->arch.pv_vcpu.need_cr0_wp_set = 1;
>>
>>PV field access within a non-PV-only code block?
>>
>>> +                }
>>
>>I wonder how well the window where you're running with CR0.WP clear
>>is bounded: The flag serves as a kind of security measure, and hence
>>shouldn't be left off for extended periods of time.
> 
> I agree. Is there a way I can bound this?

That's what you need to figure out. The simplistic solution (single
stepping just the critical instruction(s)) is probably not going to be
acceptable due to its fragility. I have no good other suggestions,
but I'm not eager to allow code in that weakens protection.

>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -43,6 +43,7 @@
>>>  #include <asm/page.h>
>>>  #include <asm/numa.h>
>>>  #include <asm/flushtlb.h>
>>> +#include <asm/shadow.h>
>>
>>Breaking ARM?
>>>  #ifdef CONFIG_X86
>>>  #include <asm/p2m.h>
>>>  #include <asm/setup.h> /* for highmem_start only */
>>> @@ -1660,6 +1661,8 @@ int assign_pages(
>>>          page_set_owner(&pg[i], d);
>>>          smp_wmb(); /* Domain pointer must be visible before updating 
>>> refcnt.
>>*/
>>>          pg[i].count_info = PGC_allocated | 1;
>>> +        if ( is_pv_domain(d) )
>>> +            shadow_set_access(&pg[i], p2m_get_hostp2m(d)->default_access);
>>
>>I don't think you should call shadow code from here.
> 
> Should I add a p2m wrapper for this, so that it is valid only for x86 and a 
> no-op for ARM?

That's not necessarily enough, but at least presumably the right
route: You also need to avoid fiddling with struct page_info fields
that may be used (now or in the future) for other purposes, i.e.
you need to gate the setting of the flags by more than just
is_pv_domain().

>>> @@ -65,6 +67,11 @@ do {
>>                      \
>>>     case 8: __put_user_asm(x,ptr,retval,"q","","ir",errret);break;  \
>>>     default: __put_user_bad();                                      \
>>>     }                                                               \
>>> +    if ( unlikely(current->arch.pv_vcpu.need_cr0_wp_set) ) \
>>> +    { \
>>> +        write_cr0(read_cr0() | X86_CR0_WP); \
>>> +        current->arch.pv_vcpu.need_cr0_wp_set = 0; \
>>> +    } \
>>>  } while (0)
>>
>>Please obey to the original indentation method.
> 
> I thought tab characters were not allowed for indentation. But I guess you 
> do not want tabs and spaces for indentation purposes to be mixed within a 
> macro?

Correct - ./CODING_STYLE explicitly says that you should match
existing coding style when an entire file (or a significant code
portion) was imported from elsewhere without style adjustment.

Jan

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