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:
>> --- 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.
> 
> 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()).
> 
> Or, if we don't want to crash on a production box in that case, we
> should leave the if() statement there.

Considering

    ASSERT(mfn_valid(gmfn));

in sh_remove_shadows(), rather than leaving the conditional,
wouldn't it then be better to drop the ASSERT() and use

        if ( VALID_M2P(gmfn) )
            shadow_remove_all_shadows(owner, _mfn(gmfn));

?

Jan


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