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, <gsco...@alpha.ncsc.mil>
> + *
> + * Contributors: Michael LeMay, <mdle...@epoch.ncsc.mil>
> + *
> + * 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
