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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.18 at 11:22, <talons.lee@xxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -899,6 +899,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 | flask`
> +
> +> 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.  Basic restriction for common 
> deployment
> +  (the dummy module) will be applied.  it's also used when XSM is compiled 
> out.

"It's" (upper case I).

> +* `flask`: this is the policy based access control.  To choose this, the
> +  separated option in kconfig must also be enabled.

Perhaps better "separate" (but I'm not a native speaker)?

> @@ -154,6 +154,17 @@ config XSM_FLASK_POLICY
>  
>         If unsure, say Y.
>  
> +choice
> +     prompt "Default XSM implementation"
> +     depends on XSM
> +     default XSM_FLASK_DEFAULT if XSM_FLASK
> +     default XSM_DUMMY_DEFAULT
> +     config XSM_DUMMY_DEFAULT
> +             bool "Match non-XSM behavior"
> +     config XSM_FLASK_DEFAULT
> +             bool "FLux Advanced Security Kernel" if XSM_FLASK
> +endchoice

Personally I'd prefer XSM_DEFAULT_*; not sure what others think.

> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -31,6 +31,37 @@
>  
>  struct xsm_operations *xsm_ops;
>  
> +enum xsm_bootparam {
> +    XSM_BOOTPARAM_DUMMY,
> +    XSM_BOOTPARAM_FLASK,

Considering the #ifdef-s further down, should this perhaps also be
put in "#ifdef CONFIG_XSM_FLASK", to avoid it mistakenly getting
used when the code was compiled out?

> +};
> +
> +static enum xsm_bootparam __initdata xsm_bootparam =
> +#ifdef CONFIG_XSM_FLASK_DEFAULT
> +    XSM_BOOTPARAM_FLASK;
> +#else
> +    XSM_BOOTPARAM_DUMMY;
> +#endif
> +
> +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 {

Style (brace goes on its own line).

> +        printk("XSM: can't parse boot parameter xsm=%s\n", s);

Again, not being a native speaker, "can't parse" sounds odd. Why
not just "unknown" or "unknown or unsupported"?

> @@ -57,7 +88,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:
> +        break;
> +
> +    case XSM_BOOTPARAM_FLASK:
> +        flask_init(policy_buffer, policy_size);
> +        break;
> +
> +    default:
> +        printk("XSM: Invalid value for xsm= boot parameter\n");
> +        break;

What situation is this covering, when all enumerators already have
their case label? Simply ASSERT_UNREACHABLE()?

Jan



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