LoadPin: Allow filesystem switch when not enforcing

PATCH In review Kees Cook 2 participants 6 messages Dec 9, 2022 – Dec 12, 2022

Subsystems (2)

Security (LSM) Loadpin security module

lore

Share & tools

lore

Kees Cook

author

2022-12-09 19:57 UTC #0 lore raw

cover LoadPin: Allow filesystem switch when not enforcing

Hi,

Right now, LoadPin isn't much use on general purpose distros since modules tend to be loaded from multiple filesystems at boot (first initramfs, then real rootfs). Allow the potential mount pin to move when enforcement is not enabled.

-Kees

Kees Cook (4):
LoadPin: Refactor read-only check into a helper
LoadPin: Refactor sysctl initialization
LoadPin: Move pin reporting cleanly out of locking
LoadPin: Allow filesystem switch when not enforcing

 security/loadpin/loadpin.c | 89 ++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 37 deletions(-)

Signature · 1 line

2.34.1

Kees Cook

author

2022-12-09 19:57 UTC #1 lore raw

PATCH 1/4 LoadPin: Refactor read-only check into a helper

In preparation for allowing mounts to shift when not enforced, move read-only checking into a separate helper.

Cc Paul Moore <[email protected]>

Cc James Morris <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

security/loadpin/loadpin.c +21 ~~-12~~

index 110a5ab2b46b..ca0eff3ce9d0 100644
@@ -72,28 +72,21 @@ static struct ctl_table loadpin_sysctl_table[] = {
 	{ }
 };
 
-/*
- * This must be called after early kernel init, since then the rootdev
- * is available.
- */
-static void check_pinning_enforcement(struct super_block *mnt_sb)
+static void report_writable(struct super_block *mnt_sb, bool writable)
 {
-	bool ro = false;
-
 	/*
 	 * If load pinning is not enforced via a read-only block
 	 * device, allow sysctl to change modes for testing.
 	 */
 	if (mnt_sb->s_bdev) {
-		ro = bdev_read_only(mnt_sb->s_bdev);
 		pr_info("%pg (%u:%u): %s\n", mnt_sb->s_bdev,
 			MAJOR(mnt_sb->s_bdev->bd_dev),
 			MINOR(mnt_sb->s_bdev->bd_dev),
-			ro ? "read-only" : "writable");
+			writable ? "writable" : "read-only");
 	} else
 		pr_info("mnt_sb lacks block device, treating as: writable\n");
 
-	if (!ro) {
+	if (writable) {
 		if (!register_sysctl_paths(loadpin_sysctl_path,
 					   loadpin_sysctl_table))
 			pr_notice("sysctl registration failed!\n");

@@ -103,12 +96,26 @@ static void check_pinning_enforcement(struct super_block *mnt_sb)
 		pr_info("load pinning engaged.\n");
 }
 #else
-static void check_pinning_enforcement(struct super_block *mnt_sb)
+static void report_writable(struct super_block *mnt_sb, bool writable)
 {
 	pr_info("load pinning engaged.\n");
 }
 #endif
 
+/*
+ * This must be called after early kernel init, since then the rootdev
+ * is available.
+ */
+static bool sb_is_writable(struct super_block *mnt_sb)
+{
+	bool writable = true;
+
+	if (mnt_sb->s_bdev)
+		writable = !bdev_read_only(mnt_sb->s_bdev);
+
+	return writable;
+}
+
 static void loadpin_sb_free_security(struct super_block *mnt_sb)
 {
 	/*

@@ -126,6 +133,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 {
 	struct super_block *load_root;
 	const char *origin = kernel_read_file_id_str(id);
+	bool load_root_writable;
 
 	/* If the file id is excluded, ignore the pinning. */
 	if ((unsigned int)id < ARRAY_SIZE(ignore_read_file_id) &&

@@ -146,6 +154,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 	}
 
 	load_root = file->f_path.mnt->mnt_sb;
+	load_root_writable = sb_is_writable(load_root);
 
 	/* First loaded module/firmware defines the root for all others. */
 	spin_lock(&pinned_root_spinlock);

@@ -162,7 +171,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 		 * enforcing. This would be purely cosmetic.
 		 */
 		spin_unlock(&pinned_root_spinlock);
-		check_pinning_enforcement(pinned_root);
+		report_writable(pinned_root, load_root_writable);
 		report_load(origin, file, "pinned");
 	} else {
 		spin_unlock(&pinned_root_spinlock);

Signature · 1 line

2.34.1

Kees Cook

author

2022-12-09 19:57 UTC #2 lore raw

PATCH 2/4 LoadPin: Refactor sysctl initialization

In reply to Kees Cook

In preparation for shifting root mount when not enforcing, split sysctl logic out into a separate helper, and unconditionally register the sysctl, but only make it writable when the device is writable.

Cc Paul Moore <[email protected]>

Cc James Morris <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

security/loadpin/loadpin.c +19 ~~-16~~

index ca0eff3ce9d0..5b15f8f7268d 100644
@@ -52,7 +52,6 @@ static bool deny_reading_verity_digests;
 #endif
 
 #ifdef CONFIG_SYSCTL
-
 static struct ctl_path loadpin_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "loadpin", },

@@ -66,18 +65,29 @@ static struct ctl_table loadpin_sysctl_table[] = {
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
 		.proc_handler   = proc_dointvec_minmax,
-		.extra1         = SYSCTL_ZERO,
+		.extra1         = SYSCTL_ONE,
 		.extra2         = SYSCTL_ONE,
 	},
 	{ }
 };
 
-static void report_writable(struct super_block *mnt_sb, bool writable)
+static void set_sysctl(bool is_writable)
 {
 	/*
 	 * If load pinning is not enforced via a read-only block
 	 * device, allow sysctl to change modes for testing.
 	 */
+	if (is_writable)
+		loadpin_sysctl_table[0].extra1 = SYSCTL_ZERO;
+	else
+		loadpin_sysctl_table[0].extra1 = SYSCTL_ONE;
+}
+#else
+static inline void set_sysctl(bool is_writable) { }
+#endif
+
+static void report_writable(struct super_block *mnt_sb, bool writable)
+{
 	if (mnt_sb->s_bdev) {
 		pr_info("%pg (%u:%u): %s\n", mnt_sb->s_bdev,
 			MAJOR(mnt_sb->s_bdev->bd_dev),

@@ -86,21 +96,9 @@ static void report_writable(struct super_block *mnt_sb, bool writable)
 	} else
 		pr_info("mnt_sb lacks block device, treating as: writable\n");
 
-	if (writable) {
-		if (!register_sysctl_paths(loadpin_sysctl_path,
-					   loadpin_sysctl_table))
-			pr_notice("sysctl registration failed!\n");
-		else
-			pr_info("enforcement can be disabled.\n");
-	} else
+	if (!writable)
 		pr_info("load pinning engaged.\n");
 }
-#else
-static void report_writable(struct super_block *mnt_sb, bool writable)
-{
-	pr_info("load pinning engaged.\n");
-}
-#endif
 
 /*
  * This must be called after early kernel init, since then the rootdev

@@ -172,6 +170,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 		 */
 		spin_unlock(&pinned_root_spinlock);
 		report_writable(pinned_root, load_root_writable);
+		set_sysctl(load_root_writable);
 		report_load(origin, file, "pinned");
 	} else {
 		spin_unlock(&pinned_root_spinlock);

@@ -259,6 +258,10 @@ static int __init loadpin_init(void)
 	pr_info("ready to pin (currently %senforcing)\n",
 		enforce ? "" : "not ");
 	parse_exclude();
+#ifdef CONFIG_SYSCTL
+	if (!register_sysctl_paths(loadpin_sysctl_path, loadpin_sysctl_table))
+		pr_notice("sysctl registration failed!\n");
+#endif
 	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
 
 	return 0;

Signature · 1 line

2.34.1

Kees Cook

author

2022-12-09 19:57 UTC #3 lore raw

PATCH 3/4 LoadPin: Move pin reporting cleanly out of locking

In reply to Kees Cook

Refactor the pin reporting to be more cleanly outside the locking. It was already, but moving it around helps clear the path for the root to switch when not enforcing.

Cc Paul Moore <[email protected]>

Cc James Morris <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

security/loadpin/loadpin.c +6 -9

index 5b15f8f7268d..ef12d77548ae 100644
@@ -131,6 +131,7 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 {
 	struct super_block *load_root;
 	const char *origin = kernel_read_file_id_str(id);
+	bool first_root_pin = false;
 	bool load_root_writable;
 
 	/* If the file id is excluded, ignore the pinning. */

@@ -162,18 +163,14 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 	 */
 	if (!pinned_root) {
 		pinned_root = load_root;
-		/*
-		 * Unlock now since it's only pinned_root we care about.
-		 * In the worst case, we will (correctly) report pinning
-		 * failures before we have announced that pinning is
-		 * enforcing. This would be purely cosmetic.
-		 */
-		spin_unlock(&pinned_root_spinlock);
+		first_root_pin = true;
+	}
+	spin_unlock(&pinned_root_spinlock);
+
+	if (first_root_pin) {
 		report_writable(pinned_root, load_root_writable);
 		set_sysctl(load_root_writable);
 		report_load(origin, file, "pinned");
-	} else {
-		spin_unlock(&pinned_root_spinlock);
 	}
 
 	if (IS_ERR_OR_NULL(pinned_root) ||

Signature · 1 line

2.34.1

Kees Cook

author

2022-12-09 19:58 UTC #4 lore raw

PATCH 4/4 LoadPin: Allow filesystem switch when not enforcing

In reply to Kees Cook

For LoadPin to be used at all in a classic distro environment, it needs to allow for switching filesystems (from the initramfs to the "real" root filesystem). To allow for this, if the "enforce" mode is not set at boot, reset the pinned filesystem tracking when the pinned filesystem gets unmounted instead of invalidating further loads. Once enforcement is set, it cannot be unset, and the pinning will stick.

This means that distros can build with CONFIG_SECURITY_LOADPIN=y, but with CONFIG_SECURITY_LOADPIN_ENFORCE disabled, but after boot is running, the system can enable enforcement:

  $ sysctl -w kernel.loadpin.enforced=1

Cc Paul Moore <[email protected]>

Cc James Morris <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

security/loadpin/loadpin.c +11 -5

index ef12d77548ae..d73a281adf86 100644
@@ -119,11 +119,16 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb)
 	/*
 	 * When unmounting the filesystem we were using for load
 	 * pinning, we acknowledge the superblock release, but make sure
-	 * no other modules or firmware can be loaded.
+	 * no other modules or firmware can be loaded when we are in
+	 * enforcing mode. Otherwise, allow the root to be reestablished.
 	 */
 	if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) {
-		pinned_root = ERR_PTR(-EIO);
-		pr_info("umount pinned fs: refusing further loads\n");
+		if (enforce) {
+			pinned_root = ERR_PTR(-EIO);
+			pr_info("umount pinned fs: refusing further loads\n");
+		} else {
+			pinned_root = NULL;
+		}
 	}
 }

@@ -158,8 +163,9 @@ static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 	/* First loaded module/firmware defines the root for all others. */
 	spin_lock(&pinned_root_spinlock);
 	/*
-	 * pinned_root is only NULL at startup. Otherwise, it is either
-	 * a valid reference, or an ERR_PTR.
+	 * pinned_root is only NULL at startup or when the pinned root has
+	 * been unmounted while we are not in enforcing mode. Otherwise, it
+	 * is either a valid reference, or an ERR_PTR.
 	 */
 	if (!pinned_root) {
 		pinned_root = load_root;

Signature · 1 line

2.34.1

Serge E. Hallyn

maintainer Reviewed-by on the v1 series

2022-12-12 21:32 UTC #5 lore raw

In reply to Kees Cook

Quoted 8 lines · Kees Cook

Hi,
Right now, LoadPin isn't much use on general purpose distros since modules tend to be loaded from multiple filesystems at boot (first initramfs, then real rootfs). Allow the potential mount pin to move when enforcement is not enabled.
-Kees

Reviewed-by Serge Hallyn <[email protected]>

to the set, thanks.

Quoted 12 lines

Kees Cook (4):
LoadPin: Refactor read-only check into a helper
LoadPin: Refactor sysctl initialization
LoadPin: Move pin reporting cleanly out of locking
LoadPin: Allow filesystem switch when not enforcing
 security/loadpin/loadpin.c | 89 ++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 37 deletions(-)
Signature · 1 line
2.34.1