Re: [Xen-devel] [PATCH v2 29/30] xen/x86: allow PVHv2 to perform foreign memory mappings

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/10/16 15:50, Jan Beulich wrote:
>>>> On 10.10.16 at 16:27, <george.dunlap@xxxxxxxxxx> wrote:
>> On 10/10/16 15:21, Jan Beulich wrote:
>>>>>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned 
>>>> long fgfn,
>>>>      struct domain *fdom;
>>>>  
>>>>      ASSERT(tdom);
>>>> -    if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) )
>>>> +    if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) )
>>>>          return -EINVAL;
>>>
>>> Can PV domains make it here? If not, dropping the predicate would
>>> seem the better adjustment.
>>
>> Is there any chance that in the future PV domains might accidentally get
>> here because of some other changes in the future?  If so, leaving the
>> predicate seems like a sensible precaution to reduce the risk of XSAs
>> down the road, since we're doing a load of checking already anyway. ;-)
> 
> In which case I'd still prefer to extend the ASSERT() right ahead of
> the if(). But you're the maintainer of this code, so you know best.

ASSERT()s and BUG_ON()s are not suitable for checks with security
implications.  Unless we specifically test for PV guests making this
hypercall, both are very likely to slip entirely through any testing we
do; in which case the former would become a full security issue, and the
latter becomes a DoS.

 -George


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