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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 27 Jan 2022, Julien Grall wrote:
> On 27/01/2022 12:07, Jan Beulich wrote:
> > 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.
> 
> This would not help for real XSM policy. We would either need to bypass XSM
> completely or decide what to do depending on the mode (e.g. SILO, FLASK...).
> 
> > 
> > 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.
> 
> The problem is you will end up to modify a lot of code. This will be quite
> tedious when the policy is likely going to be the same (e.g. if we are
> booting, then allow to use the hypercall).
> 
> So I think it makes more sense to do the modification at central place. That
> said, this is not strictly necessary for what Stefano needs. So I am OK if we
> go with local hack and deferred the debate to when a wider solution is needed.

OK, thank you. I'll go with the following then.


diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..dde85444fe 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -284,7 +284,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm)
 {
     struct evtchn *chn;
     struct domain *d;
@@ -301,9 +301,12 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t 
*alloc)
         ERROR_EXIT_DOM(port, d);
     chn = evtchn_from_port(d, port);
 
-    rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
-    if ( rc )
-        goto out;
+    if ( !disable_xsm )
+    {
+        rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
+        if ( rc )
+            goto out;
+    }
 
     evtchn_write_lock(chn);
 
@@ -1195,7 +1198,7 @@ long do_event_channel_op(int cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_alloc_unbound alloc_unbound;
         if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_alloc_unbound(&alloc_unbound);
+        rc = evtchn_alloc_unbound(&alloc_unbound, false);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..5ea3ac345b 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);