Re: [Xen-devel] [PATCH v2 03/21] xen: allow console_io hypercalls from certain DomUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 17 Jul 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 17/07/2018 21:05, Stefano Stabellini wrote:
> > On Mon, 9 Jul 2018, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 07/07/18 00:11, Stefano Stabellini wrote:
> > > > Introduce an is_console option to allow certain classes of domUs to use
> > > > the Xen console. Specifically, it will be used to give console access to
> > > > all domUs started from Xen from information on device tree.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > CC: andrew.cooper3@xxxxxxxxxx
> > > > CC: George.Dunlap@xxxxxxxxxxxxx
> > > > CC: ian.jackson@xxxxxxxxxxxxx
> > > > CC: jbeulich@xxxxxxxx
> > > > CC: konrad.wilk@xxxxxxxxxx
> > > > CC: tim@xxxxxxx
> > > > CC: wei.liu2@xxxxxxxxxx
> > > > CC: dgdegra@xxxxxxxxxxxxx
> > > > ---
> > > > Changes in v2:
> > > > - introduce is_console
> > > > - remove #ifdefs
> > > > ---
> > > >    xen/include/xen/sched.h | 2 ++
> > > >    xen/include/xsm/dummy.h | 2 ++
> > > >    xen/xsm/flask/hooks.c   | 5 ++++-
> > > >    3 files changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > > index 99d2af2..d66cec0 100644
> > > > --- a/xen/include/xen/sched.h
> > > > +++ b/xen/include/xen/sched.h
> > > > @@ -379,6 +379,8 @@ struct domain
> > > >        bool             auto_node_affinity;
> > > >        /* Is this guest fully privileged (aka dom0)? */
> > > >        bool             is_privileged;
> > > > +    /* Can this guest access the Xen console? */
> > > > +    bool             is_console;
> > > >        /* Is this a xenstore domain (not dom0)? */
> > > >        bool             is_xenstore;
> > > >        /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
> > > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> > > > index ff6b2db..3888817 100644
> > > > --- a/xen/include/xsm/dummy.h
> > > > +++ b/xen/include/xsm/dummy.h
> > > > @@ -230,6 +230,8 @@ static XSM_INLINE int
> > > > xsm_memory_stat_reservation(XSM_DEFAULT_ARG struct domain
> > > >    static XSM_INLINE int xsm_console_io(XSM_DEFAULT_ARG struct domain
> > > > *d, int
> > > > cmd)
> > > >    {
> > > >        XSM_ASSERT_ACTION(XSM_OTHER);
> > > > +    if ( d->is_console )
> > > > +        return xsm_default_action(XSM_HOOK, d, NULL);
> > > 
> > > I will let Daniel commenting on this change. However ...
> > > 
> > > >    #ifdef CONFIG_VERBOSE_DEBUG
> > > >        if ( cmd == CONSOLEIO_write )
> > > >            return xsm_default_action(XSM_HOOK, d, NULL);
> > > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> > > > index 78bc326..2551e4e 100644
> > > > --- a/xen/xsm/flask/hooks.c
> > > > +++ b/xen/xsm/flask/hooks.c
> > > > @@ -443,7 +443,10 @@ static int flask_console_io(struct domain *d, int
> > > > cmd)
> > > >            return avc_unknown_permission("console_io", cmd);
> > > >        }
> > > >    -    return domain_has_xen(d, perm);
> > > > +    if ( !d->is_console )
> > > > +        return domain_has_xen(d, perm);
> > > > +    else
> > > > +        return 0;
> > > 
> > > ... I don't think this change is correct. When a policy is used, the user
> > > is
> > > free to define what is the behavior. With your solution, you impose the
> > > console access even if the user didn't to not give the permission.
> > 
> > I was hoping Daniel would advise on the best way to do things here.
> > 
> > I thought that the idea was that granting a domain "is_console" is
> > equivalent to granting a domain XEN__READCONSOLE and XEN__WRITECONSOLE
> > permissions.  Thus, if is_console is set, we return 0 from
> > flask_console_io because the permissions check succeeds.
> 
> Well, yes and no. That's equivalent when you use the dummy policy. When you
> have a flask policy you want to give the control to the user.
> 
> If you look at the code there are no such as d->is_privilege in that function.
> This means that the user define the policy for the hardware domain. Why would
> be d->is_console different here?

You are saying that in hooks.c the check should remain exactly as is:

  return domain_has_xen(d, perm);

and d->is_console should not be tested? In that case, do you know if I
need to do anything special with XEN__READCONSOLE and XEN__WRITECONSOLE
permissions for the initial boot domains (such as adding those
permissions as the same time d->is_console is set)?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel