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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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)

> +
> +/* 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 ...

> +#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.

~Andrew