Re: [Xen-devel] [PATCH 5/6] x86/mm: clean up SHARED_M2P{, _ENTRY} uses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.12.17 at 18:50, <george.dunlap@xxxxxxxxxx> wrote:
> On 12/12/2017 03:08 PM, Jan Beulich wrote:
>> Stop open-coding SHARED_M2P() and drop a pointless use of it from
>> paging_mfn_is_dirty() (!VALID_M2P() is a superset of SHARED_M2P()) and
>> another one from free_page_type() (prior assertions render this
>> redundant).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2371,9 +2371,7 @@ int free_page_type(struct page_info *pag
>>  
>>          gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
>>          ASSERT(VALID_M2P(gmfn));
>> -        /* Page sharing not supported for shadowed domains */
>> -        if(!SHARED_M2P(gmfn))
>> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
>> +        shadow_remove_all_shadows(owner, _mfn(gmfn));
> 
> But that's an ASSERT(), not a BUG_ON().  Code after an ASSERT() needs to
> make sure that if it turns out to be false in a non-debug run, nothing
> worse than a BUG() will happen -- for instance, an information leak or a
> privilege escalation.

Okay, I think I finally will need to introduce the assert-or-crash-
domain construct as a prereq here. I agree with the general
comment you make, however we have lots and lots of examples
to the contrary, not the least ...

> xen/arch/x86/mm/shadow/common.c:sh_remove_shadows() looks up the page
> struct for the mfn without checking if it's valid; so it will *probably*
> end up accessing a wild pointer; at which point it would be better to
> change the ASSERT(VALID_M2P()) into a BUG_ON(!VALID_M2P()).

... sh_remove_shadows() with its "ASSERT(mfn_valid(gmfn))",
which is the very sanity check allowing the looked up page
pointer to be de-referenced.

>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -369,8 +369,8 @@ int paging_mfn_is_dirty(struct domain *d
>>  
>>      /* We /really/ mean PFN here, even for non-translated guests. */
>>      pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
>> -    /* Shared pages are always read-only; invalid pages can't be dirty. */
>> -    if ( unlikely(SHARED_M2P(pfn_x(pfn)) || !VALID_M2P(pfn_x(pfn))) )
>> +    /* Invalid pages can't be dirty. */
>> +    if ( unlikely(!VALID_M2P(pfn_x(pfn))) )
>>          return 0;
> 
> Are you sure that it will always be the case in the future that
> SHARED_MP2(x) implies !VALID_M2P(x)?  (This is also relevant for my
> previous comment.)

Well, at least with the current concept it always will be afaict. As
for its relevance to the previous comment: In the description I'm
specifically mentioning paging_mfn_is_dirty() but not
free_page_type() - in the latter the SHARED_M2P() check isn't
being dropped for being redundant with VALID_M2P(), but for
being dead code altogether (due to the earlier
ASSERT(!shadow_mode_refcounts(owner))).

Of course both ASSERT()s there suffer the same problem as
mentioned above. I doubt it should be the subject of this patch
to convert all of them to the to-be-introduced construct, despite
them sitting in code next to one being modified.

Jan


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