linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] LoadPin: Allow filesystem switch when not enforcing
@ 2021-04-08 23:28 Kees Cook
  2021-04-10 18:02 ` kernel test robot
  2021-04-11  6:02 ` kernel test robot
  0 siblings, 2 replies; 3+ messages in thread
From: Kees Cook @ 2021-04-08 23:28 UTC (permalink / raw)
  To: linux-kernel, linux-security-module; +Cc: 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). If the "enforce" mode is not set, reset the pinned
filesystem tracking when the pinned filesystem gets unmounted.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/loadpin/loadpin.c | 110 +++++++++++++++++++++----------------
 1 file changed, 63 insertions(+), 47 deletions(-)

diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index b12f7d986b1e..ca1bbfe4a44b 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -45,7 +45,6 @@ static struct super_block *pinned_root;
 static DEFINE_SPINLOCK(pinned_root_spinlock);
 
 #ifdef CONFIG_SYSCTL
-
 static struct ctl_path loadpin_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "loadpin", },
@@ -59,69 +58,81 @@ 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,
 	},
 	{ }
 };
 
-/*
- * 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 set_sysctl(bool is_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) {
-		char bdev[BDEVNAME_SIZE];
-
-		ro = bdev_read_only(mnt_sb->s_bdev);
-		bdevname(mnt_sb->s_bdev, bdev);
-		pr_info("%s (%u:%u): %s\n", bdev,
-			MAJOR(mnt_sb->s_bdev->bd_dev),
-			MINOR(mnt_sb->s_bdev->bd_dev),
-			ro ? "read-only" : "writable");
-	} else
-		pr_info("mnt_sb lacks block device, treating as: writable\n");
-
-	if (!ro) {
-		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
-		pr_info("load pinning engaged.\n");
+	if (is_writable)
+		loadpin_sysctl_table[0].extra1 = SYSCTL_ZERO;
+	else
+		loadpin_sysctl_table[0].extra1 = SYSCTL_ONE;
 }
 #else
-static void check_pinning_enforcement(struct super_block *mnt_sb)
+static bool set_sysctl(bool is_writable) { }
+#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, struct block_device **bdev)
+{
+	bool writable = true;
+
+	*bdev = mnt_sb->s_bdev;
+	if (*bdev)
+		writable = !bdev_read_only(*bdev);
+
+	return writable;
+}
+
+static void report_writable(struct block_device *bdev)
 {
-	pr_info("load pinning engaged.\n");
+	if (bdev) {
+		char name[BDEVNAME_SIZE];
+
+		bdevname(bdev, name);
+		pr_info("%s (%u:%u): %s\n", name,
+			MAJOR(bdev->bd_dev),
+			MINOR(bdev->bd_dev),
+			load_root_writable ? "writable" : "read-only");
+	} else {
+		pr_info("pinned filesystem lacks block device, treating as: writable\n");
+	}
 }
-#endif
 
 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 (enforced) {
+			pinned_root = ERR_PTR(-EIO);
+			pr_info("umount pinned fs: refusing further loads\n");
+		} else {
+			pinned_root = NULL;
+		}
 	}
 }
 
 static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
 			     bool contents)
 {
+	struct block_device *bdev = NULL;
 	struct super_block *load_root;
+	bool load_root_writable, first_root_pin, sysctl_needed;
 	const char *origin = kernel_read_file_id_str(id);
 
 	/*
@@ -152,26 +163,27 @@ static int loadpin_read_file(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, &bdev);
+	sysctl_needed = false;
+	first_root_pin = false;
 
 	/* 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;
-		/*
-		 * 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);
-		check_pinning_enforcement(pinned_root);
+		first_root_pin = true;
+	}
+	spin_unlock(&pinned_root_spinlock);
+
+	if (first_root_pin) {
+		report_writable(bdev);
+		set_sysctl(load_root_writable);
 		report_load(origin, file, "pinned");
-	} else {
-		spin_unlock(&pinned_root_spinlock);
 	}
 
 	if (IS_ERR_OR_NULL(pinned_root) || load_root != pinned_root) {
@@ -239,6 +251,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;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] LoadPin: Allow filesystem switch when not enforcing
  2021-04-08 23:28 [PATCH] LoadPin: Allow filesystem switch when not enforcing Kees Cook
@ 2021-04-10 18:02 ` kernel test robot
  2021-04-11  6:02 ` kernel test robot
  1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-04-10 18:02 UTC (permalink / raw)
  To: Kees Cook, linux-kernel, linux-security-module
  Cc: kbuild-all, clang-built-linux, Kees Cook

[-- Attachment #1: Type: text/plain, Size: 3622 bytes --]

Hi Kees,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on kees/for-next/pstore linus/master v5.12-rc6 next-20210409]
[cannot apply to kees/for-next/loadpin]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/LoadPin-Allow-filesystem-switch-when-not-enforcing/20210409-073059
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5e46d1b78a03d52306f21f77a4e4a144b6d31486
config: x86_64-randconfig-a003-20210410 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project dd453a1389b6a7e6d9214b449d3c54981b1a89b6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/3dc7289d9d15396745929884191874dc2cce1afc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kees-Cook/LoadPin-Allow-filesystem-switch-when-not-enforcing/20210409-073059
        git checkout 3dc7289d9d15396745929884191874dc2cce1afc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> security/loadpin/loadpin.c:106:4: error: use of undeclared identifier 'load_root_writable'
                           load_root_writable ? "writable" : "read-only");
                           ^
>> security/loadpin/loadpin.c:121:7: error: use of undeclared identifier 'enforced'; did you mean 'enforce'?
                   if (enforced) {
                       ^~~~~~~~
                       enforce
   security/loadpin/loadpin.c:41:12: note: 'enforce' declared here
   static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
              ^
   2 errors generated.


vim +/load_root_writable +106 security/loadpin/loadpin.c

    96	
    97	static void report_writable(struct block_device *bdev)
    98	{
    99		if (bdev) {
   100			char name[BDEVNAME_SIZE];
   101	
   102			bdevname(bdev, name);
   103			pr_info("%s (%u:%u): %s\n", name,
   104				MAJOR(bdev->bd_dev),
   105				MINOR(bdev->bd_dev),
 > 106				load_root_writable ? "writable" : "read-only");
   107		} else {
   108			pr_info("pinned filesystem lacks block device, treating as: writable\n");
   109		}
   110	}
   111	
   112	static void loadpin_sb_free_security(struct super_block *mnt_sb)
   113	{
   114		/*
   115		 * When unmounting the filesystem we were using for load
   116		 * pinning, we acknowledge the superblock release, but make sure
   117		 * no other modules or firmware can be loaded when we are in
   118		 * enforcing mode. Otherwise, allow the root to be reestablished.
   119		 */
   120		if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) {
 > 121			if (enforced) {
   122				pinned_root = ERR_PTR(-EIO);
   123				pr_info("umount pinned fs: refusing further loads\n");
   124			} else {
   125				pinned_root = NULL;
   126			}
   127		}
   128	}
   129	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37717 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] LoadPin: Allow filesystem switch when not enforcing
  2021-04-08 23:28 [PATCH] LoadPin: Allow filesystem switch when not enforcing Kees Cook
  2021-04-10 18:02 ` kernel test robot
@ 2021-04-11  6:02 ` kernel test robot
  1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-04-11  6:02 UTC (permalink / raw)
  To: Kees Cook, linux-kernel, linux-security-module; +Cc: kbuild-all, Kees Cook

[-- Attachment #1: Type: text/plain, Size: 4304 bytes --]

Hi Kees,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on kees/for-next/pstore linus/master v5.12-rc6 next-20210409]
[cannot apply to kees/for-next/loadpin]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/LoadPin-Allow-filesystem-switch-when-not-enforcing/20210409-073059
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 5e46d1b78a03d52306f21f77a4e4a144b6d31486
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/3dc7289d9d15396745929884191874dc2cce1afc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kees-Cook/LoadPin-Allow-filesystem-switch-when-not-enforcing/20210409-073059
        git checkout 3dc7289d9d15396745929884191874dc2cce1afc
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:16,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from security/loadpin/loadpin.c:12:
   security/loadpin/loadpin.c: In function 'report_writable':
>> security/loadpin/loadpin.c:106:4: error: 'load_root_writable' undeclared (first use in this function)
     106 |    load_root_writable ? "writable" : "read-only");
         |    ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:373:34: note: in definition of macro 'pr_info'
     373 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
         |                                  ^~~~~~~~~~~
   security/loadpin/loadpin.c:106:4: note: each undeclared identifier is reported only once for each function it appears in
     106 |    load_root_writable ? "writable" : "read-only");
         |    ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:373:34: note: in definition of macro 'pr_info'
     373 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
         |                                  ^~~~~~~~~~~
   security/loadpin/loadpin.c: In function 'loadpin_sb_free_security':
>> security/loadpin/loadpin.c:121:7: error: 'enforced' undeclared (first use in this function); did you mean 'enforce'?
     121 |   if (enforced) {
         |       ^~~~~~~~
         |       enforce
   security/loadpin/loadpin.c: In function 'loadpin_read_file':
   security/loadpin/loadpin.c:135:43: warning: variable 'sysctl_needed' set but not used [-Wunused-but-set-variable]
     135 |  bool load_root_writable, first_root_pin, sysctl_needed;
         |                                           ^~~~~~~~~~~~~


vim +/load_root_writable +106 security/loadpin/loadpin.c

    96	
    97	static void report_writable(struct block_device *bdev)
    98	{
    99		if (bdev) {
   100			char name[BDEVNAME_SIZE];
   101	
   102			bdevname(bdev, name);
   103			pr_info("%s (%u:%u): %s\n", name,
   104				MAJOR(bdev->bd_dev),
   105				MINOR(bdev->bd_dev),
 > 106				load_root_writable ? "writable" : "read-only");
   107		} else {
   108			pr_info("pinned filesystem lacks block device, treating as: writable\n");
   109		}
   110	}
   111	
   112	static void loadpin_sb_free_security(struct super_block *mnt_sb)
   113	{
   114		/*
   115		 * When unmounting the filesystem we were using for load
   116		 * pinning, we acknowledge the superblock release, but make sure
   117		 * no other modules or firmware can be loaded when we are in
   118		 * enforcing mode. Otherwise, allow the root to be reestablished.
   119		 */
   120		if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) {
 > 121			if (enforced) {
   122				pinned_root = ERR_PTR(-EIO);
   123				pr_info("umount pinned fs: refusing further loads\n");
   124			} else {
   125				pinned_root = NULL;
   126			}
   127		}
   128	}
   129	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65020 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-11  6:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 23:28 [PATCH] LoadPin: Allow filesystem switch when not enforcing Kees Cook
2021-04-10 18:02 ` kernel test robot
2021-04-11  6:02 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).