Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

["Xin Li (Talons)" <xin.li@xxxxxxxxxx>]

Hell Andrew,

> -----Original Message-----
> From: Andrew Cooper
> Sent: Friday, June 29, 2018 5:48 PM
> To: Xin Li <talons.lee@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: Xin Li (Talons) <xin.li@xxxxxxxxxx>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan
> Beulich <JBeulich@xxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Sergey Dyasli
> <sergey.dyasli@xxxxxxxxxx>; Ming Lu <ming.lu@xxxxxxxxxx>
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> 
> On 29/06/18 10:28, Xin Li wrote:
> > Introduce new boot parameter xsm to choose which xsm module is
> > enabled, and set default to dummy.
> >
> > Signed-off-by: Xin Li <xin.li@xxxxxxxxxx>
> 
> As a note for other reviewers, this series is based on top of my XSM Kconfig
> cleanup.
> 
> As for this patch, its almost there.  Just a few minor issues.
> 
> >
> > ---
> > CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> > CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > CC: Tim Deegan <tim@xxxxxxx>
> > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Ming Lu <ming.lu@xxxxxxxxxx>
> > ---
> >  docs/misc/xen-command-line.markdown | 13 ++++++++++
> >  xen/xsm/xsm_core.c                  | 39 ++++++++++++++++++++++++++++-
> >  2 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index 075e5ea159..7c689b8225 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> >  Note that specifying zero as domU value means zero, while for dom0 it
> > means  to use the default.
> >
> > +### xsm
> > +> `= dummy | silo | flask`
> 
> This should be just "dummy | flask" in this patch, and extended with silo in 
> the
> next path.  Also, options in this file should be sorted alphabetically, so 
> ### xsm
> should be near the end, rather than beside flask.

Right. Done.

> 
> > +
> > +> Default: `dummy`
> > +
> > +Specify which XSM module should be enabled.  This option is only
> > +available if the hypervisor was compiled with XSM support.
> > +
> > +* `dummy`: this is the default choice.  No special restriction will be 
> > applied.
> > +  it's also used when XSM is compiled out.
> > +* `flask`: this is the policy based access control.  To choose this,
> > +the
> > +  separated option in kconfig must also be enabled.
> > +
> >  ### flask
> >  > `= permissive | enforcing | late | disabled`
> >
> > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index
> > cddcf7aa51..e002200578 100644
> > --- a/xen/xsm/xsm_core.c
> > +++ b/xen/xsm/xsm_core.c
> > @@ -31,6 +31,30 @@
> >
> >  struct xsm_operations *xsm_ops;
> >
> > +enum xsm_bootparam {
> > +    XSM_BOOTPARAM_DUMMY,
> > +    XSM_BOOTPARAM_FLASK,
> > +    XSM_BOOTPARAM_INVALID,
> 
> I'd drop INVALID (See below for the parsing aspect), as it actually falls 
> back to
> DUMMY.
> 
> > +};
> > +
> > +enum xsm_bootparam __read_mostly xsm_bootparam =
> XSM_BOOTPARAM_DUMMY;
> 
> This should be __initdata rather than __read_mostly.  It is safe to be 
> discarded
> after boot.
> 
> > +
> > +static int __init parse_xsm_param(const char *s) {
> 
> int rc = 0;
> 
> > +    if ( !strcmp(s, "dummy") )
> > +        xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> > +    else if ( !strcmp(s, "flask") )
> > +        xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> > +    else
> > +        xsm_bootparam = XSM_BOOTPARAM_INVALID;
> > +
> > +    return 0;
> 
> else
>     rc = -EINVAL;
> 
> return rc;
> 
> As a result, the core command line infrastructure will inform the user if they
> passed an unrecognised option.
> 
OK. Thank you.

> ~Andrew
> 
> > +}
> > +
> > +custom_param("xsm", parse_xsm_param);
> > +
> >  static inline int verify(struct xsm_operations *ops)  {
> >      /* verify the security_operations structure exists */ @@ -57,7
> > +81,20 @@ static int __init xsm_core_init(const void *policy_buffer, size_t
> policy_size)
> >      }
> >
> >      xsm_ops = &dummy_xsm_ops;
> > -    flask_init(policy_buffer, policy_size);
> > +
> > +    switch ( xsm_bootparam )
> > +    {
> > +    case XSM_BOOTPARAM_DUMMY:
> > +        /* empty */
> > +        break;
> > +
> > +    case XSM_BOOTPARAM_FLASK:
> > +        flask_init(policy_buffer, policy_size);
> > +        break;
> > +
> > +    default:
> > +        printk("XSM: Invalid value for xsm= boot parameter.\n");
> > +    }
> >
> >      return 0;
> >  }



Best regards

Xin(Talons) Li
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel