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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 07/19/2018 05:19 AM, Julien Grall wrote:
> Hi Stefano,
>
> On 18/07/18 18:10, Stefano Stabellini wrote:
> > 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?
> Yes.
>
> > 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)?
> The main purpose of XSM is to provide a fine grain permission for the user to 
> configure. For instance, a user may not console access for initial domain for 
> security purpose. So you don't have anything to in the code.
>
> However, when you have XSM enabled, you will have to write down in the policy 
> that initial domains will have console access. Although, I am not sure how to 
> write that in the policy.
In a security policy that wishes to make this distinction, the initial domains 
will
have distinct security labels from domains created later.  Then, those types are
allowed console_write access (along with any other special rights they may 
need).

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