[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • Date: Tue, 7 Sep 2021 09:52:39 -0400
  • Arc-authentication-results: i=1; mx.zohomail.com; dkim=pass header.i=apertussolutions.com; spf=pass smtp.mailfrom=dpsmith@xxxxxxxxxxxxxxxxxxxx; dmarc=pass header.from=<dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1631022866; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=a0yjKN4vFjwtQ5VFXF1O0n7vkyNeAOlsxBb/XzZzYrU=; b=nwnrI5t0OWMYHJN3XD5wL4rfsmMKdxs8901CSG/lgUnqfsGJPj2J7PnTa8YjPg3UU8IFd2W5lfs7/b2HVbP5eac1J2k/M3aSGoV4Wb4JNDbhsrnlUQKqO8BX/Uxx/Z5K2JhgDknLXFeZNFMBL526sv3QXu6voz81mZbd8KMrDfQ=
  • Arc-seal: i=1; a=rsa-sha256; t=1631022866; cv=none; d=zohomail.com; s=zohoarc; b=V+msD7NyuKPbikIqyuZrQjlfG9rqnXRv+UdGyKeDJcplSHc4nfcPrXg9YnrP0zSY3xG1ALWZkkjuJu6mWHMqR8KTILHXpePtu4CG8Mw80Gkj0H0egAzg8pIzD7WkJSo50PihBB7va5WTgceXaBw1ZKq0j44hb57xabUKLD014zI=
  • Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
  • Delivery-date: Tue, 07 Sep 2021 13:54:31 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.