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

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>>>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.

From the debugging I have done to get this working, this is what the flow 
should be. Xen tries to write to guest page marked read only and page fault 
occurs. So __copy_to_user_ll() -> handle_exception_saved->do_page_fault() and 
CR0.WP is cleared. Once the fault is handled __copy_to_user_ll() is retried and 
it goes through. At the end of which CR0.WP is turned on. So this is the only 
window that pv_vcpu.need_cr0_wp_set should be true. Is there a spot outside of 
this window that I check to see if it is set and if it is, turn it back on 
again? Would that be a sufficient bound?

>>>>          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().

Coupled with your response to the other thread, I am thinking I should move 
away from using the shadow_flags for access permissions. Tim's other suggestion 
was to try and re-use the p2m-pt implementation.

>>>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.

Got it.

Thanks,
Aravindh


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