[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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



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));



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.