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
