Re: [PATCH v4 07/11] xsm: decouple xsm header inclusion selection

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 9/6/21 2:47 PM, Andrew Cooper wrote:
> On 03/09/2021 20:06, Daniel P. Smith wrote:
> > diff --git a/xen/include/xsm/xsm-core.h b/xen/include/xsm/xsm-core.h
> > new file mode 100644
> > index 0000000000..4555e111dc
> > --- /dev/null
> > +++ b/xen/include/xsm/xsm-core.h
> > @@ -0,0 +1,274 @@
> > +/*
> > + *  This file contains the XSM hook definitions for Xen.
> > + *
> > + *  This work is based on the LSM implementation in Linux 2.6.13.4.
> > + *
> > + *  Author:  George Coker, <gscoker@xxxxxxxxxxxxxx>
> > + *
> > + *  Contributors: Michael LeMay, <mdlemay@xxxxxxxxxxxxxx>
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License version 2,
> > + *  as published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __XSM_CORE_H__
> > +#define __XSM_CORE_H__
> > +
> > +#include <xen/sched.h>
> > +#include <xen/multiboot.h>
> > +
> > +/* policy magic number (defined by XSM_MAGIC) */
> > +typedef uint32_t xsm_magic_t;
> > +
> > +#ifdef CONFIG_XSM_FLASK
> > +#define XSM_MAGIC 0xf97cff8c
> > +#else
> > +#define XSM_MAGIC 0x0
> > +#endif
> Eww.  I know you're only moving code, but this construct is broken
> (right from XSM's introduction in c/s d046f361dc937), and creates a
> fairly-severe bug.
>
> It causes xsm_multiboot_policy_init() to malfunction and accept a module
> which starts with 4 zeroes, rather than the flask magic number.  The one
> caller is suitably guarded so this is only a latent bug right now, but I
> don't think we could credibly security support the code without this
> being fixed.  (Again - fine to add to the todo list.  I know there's
> loads to do)
I cannot in good conscience leave a clearly latent bug. Let me see if 
can work a safer means to handling policy module loading.
> > +
> > +/* These annotations are used by callers and in dummy.h to document the
> > + * default actions of XSM hooks. They should be compiled out otherwise.
> > + */
> For the coding style patch, this should be
>
> /*
>   * These ...
Ack.

> > +#ifdef CONFIG_XSM
> > +
> > +#ifdef CONFIG_MULTIBOOT
> > +int xsm_multiboot_init(unsigned long *module_map,
> > +                       const multiboot_info_t *mbi);
> > +int xsm_multiboot_policy_init(unsigned long *module_map,
> > +                              const multiboot_info_t *mbi,
> > +                              void **policy_buffer,
> > +                              size_t *policy_size);
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +/*
> > + * Initialize XSM
> > + *
> > + * On success, return 1 if using SILO mode else 0.
> > + */
> > +int xsm_dt_init(void);
> > +int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size);
> > +bool has_xsm_magic(paddr_t);
> > +#endif
> > +
> > +#ifdef CONFIG_XSM_FLASK
> > +const struct xsm_ops *flask_init(const void *policy_buffer,
> > +                                 size_t policy_size);
> > +#else
> > +static inline const struct xsm_ops *flask_init(const void *policy_buffer,
> > +                                               size_t policy_size)
> > +{
> > +    return NULL;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_XSM_SILO
> > +const struct xsm_ops *silo_init(void);
> > +#else
> > +static const inline struct xsm_ops *silo_init(void)
> > +{
> > +    return NULL;
> > +}
> > +#endif
> > +
> > +#else /* CONFIG_XSM */
> > +
> > +#ifdef CONFIG_MULTIBOOT
> > +static inline int xsm_multiboot_init(unsigned long *module_map,
> > +                                     const multiboot_info_t *mbi)
> > +{
> > +    return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +static inline int xsm_dt_init(void)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline bool has_xsm_magic(paddr_t start)
> > +{
> > +    return false;
> > +}
> > +#endif /* CONFIG_HAS_DEVICE_TREE */
> Shouldn't this be an #ifndef CONFIG_HAS_DEVICE_TREE ?
>
> And the answer is no because of the #else /* CONFIG_XSM */ higher up,
> but it is incredibly deceptive to read.
>
>
> I think this logic would be far easier to follow as:
>
> #if IS_ENABLED(CONFIG_XSM) && IS_ENABLED(CONFIG_MULTIBOOT)
> ...
> #else
> ...
> #endif
>
> etc.
>
> rather than having two separate #ifdef CONFIG_MULTIBOOT blocks doing
> opposite things due to the position of intermixed #ifdef CONFIG_XSM.
Ack.

v/r
dps