Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich <jbeulich@suse.com>]

On 27.01.2022 10:51, Julien Grall wrote:
> On 27/01/2022 01:50, Stefano Stabellini wrote:
>> On Wed, 26 Jan 2022, Julien Grall wrote:
>>> On 26/01/2022 07:30, Jan Beulich wrote:
>>>> On 26.01.2022 02:03, Stefano Stabellini wrote:
>>>>> Are you guys OK with something like this?
>>>>
>>>> With proper proof that this isn't going to regress anything else, maybe.
>>>
>>> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
>>> reduce the potential regression (the use of hypercall should be limited at
>>> boot).
>>>
>>> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
>>> (but in the context of Live-Update).
>>>
>>>> But ...
>>>>
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>>>>                return 0;
>>>>>            /* fall through */
>>>>>        case XSM_PRIV:
>>>>> -        if ( is_control_domain(src) )
>>>>> +        if ( is_control_domain(src) ||
>>>>> +             src->domain_id == DOMID_IDLE ||
>>>>> +             src->domain_id == DOMID_XEN )
>>>>>                return 0;
>>>>
>>>> ... my first question would be under what circumstances you might observe
>>>> DOMID_XEN here and hence why this check is there.
>>
>> For simplicity I'll answer all the points here.
>>
>> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
>> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
>> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
>> of <). The patch appended below works without issues.
>>
>> Alternatively, I am also quite happy with Jan's suggestion of passing an
>> extra parameter to evtchn_alloc_unbound, it could be:
>>
>> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
>>
>> So that XSM is enabled by default.
>>
>> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
>> having only a very minor impact.
> 
> We will likely need similar approach for other hypercalls in the future 
> if we need to call them from Xen context (e.g. when restoring domain 
> state during Live-Update).
> 
> So my preference would be to directly go with modifying the 
> xsm_default_action().

How would this help when a real XSM policy is in use? Already in SILO
mode I think you wouldn't get past the check, as the idle domain
doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
Actually I'm not even sure you'd get that far - I was under the
impression that the domain at other side of the channel may not even
be around at that time, in which case silo_evtchn_unbound() would
return -ESRCH.

Additionally relaxing things at a central place like this one comes
with the risk of relaxing too much. As said in reply to Stefano - if
this is to be done, proof will need to be provided that this doesn't
and won't permit operations which aren't supposed to be permitted. I
for one would much prefer relaxation on a case-by-case basis. That
said I'm afraid it hasn't become clear to me why the XSM check needs
bypassing here in the first place, and why this is acceptable from a
security standpoint.

>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
>>       case XSM_PRIV:
>>           if ( is_control_domain(src) )
>>               return 0;
>> +        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE )
> 
> I would surround this with unlikely and possibly prevent speculation (Jan?).

Unlikely - perhaps yes. Preventing speculation in principle also
yes, but not at the expense of adding a 2nd LFENCE (besides the one
in is_control_domain()). Yet open-coding is_control_domain() wouldn't
be very nice either. But as per above I hope anyway we're not going
to need to find a good solution here.

Jan