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, <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)
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
