Re: [RFC PATCH] loadpin: allow untoggleable enforcement on filesystems which don't appear to be read-only

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] loadpin: force_enforce fails to enable enforce and locks it to 0
--

commit dc93f122a0f976e677fefeb791538922df5a6557
Author: Amelia Crate <acrate@waldn.net>

loadpin: allow untoggleable enforcement on filesystems which don't appear to be read-only

This commit adds the loadpin.force_enforce kernel parameter and a
corresponding read-only sysctl to prevent toggling LoadPin enforcement on
filesystems that are not backed by a read-only block device.

> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index f71861f98e1ab..b260ad299a2cc 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -45,6 +45,7 @@ static void report_load(const char *origin, struct file *file, char *operation)
>  }
>  
>  static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
> +static bool force_enforce __ro_after_init;
>  static char *exclude_read_files[READING_MAX_ID];
>  static int ignore_read_file_id[READING_MAX_ID] __ro_after_init;
>  static struct super_block *pinned_root;
> @@ -60,11 +61,19 @@ static bool loadpin_root_writable;
>  static int proc_handler_loadpin(const struct ctl_table *table, int dir,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	if (!loadpin_root_writable && SYSCTL_USER_TO_KERN(dir))
> +	if ((force_enforce || !loadpin_root_writable) && SYSCTL_USER_TO_KERN(dir))
>  		return -EINVAL;
>  	return proc_dointvec_minmax(table, dir, buffer, lenp, ppos);
>  }

[Severity: High]
What happens when a system is booted with loadpin.force_enforce=1 while
enforce is 0 (for example, when CONFIG_SECURITY_LOADPIN_ENFORCE=n and
loadpin.enforce=1 is not passed on the command line, or when
loadpin.enforce=0 is passed)?

The documentation in Documentation/admin-guide/kernel-parameters.txt states
that loadpin.force_enforce=1 forces LoadPin enforcement until reboot and
only requires LoadPin to be enabled in the active LSM list, and
MODULE_PARM_DESC(force_enforce, ...) describes it as "Always enforce
module/firmware pinning".

However, force_enforce is only checked here in proc_handler_loadpin()
(under CONFIG_SYSCTL) to reject writes to /proc/sys/kernel/loadpin/enforce.
It never sets enforce = 1 in loadpin_init(), and it is not checked in
loadpin_check() or loadpin_sb_free_security():

security/loadpin/loadpin.c:loadpin_sb_free_security() {
	if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) {
		if (enforce) {
			pinned_root = ERR_PTR(-EIO);
			pr_info("umount pinned fs: refusing further loads\n");
		} else {
			pinned_root = NULL;
		}
	}
}

security/loadpin/loadpin.c:loadpin_check() {
	...
	if (IS_ERR_OR_NULL(pinned_root) ||
	    ((load_root != pinned_root) && !dm_verity_loadpin_is_bdev_trusted(load_root->s_bdev))) {
		if (unlikely(!enforce)) {
			report_load(origin, file, "pinning-ignored");
			return 0;
		}

		report_load(origin, file, "denied");
		return -EPERM;
	}
}

If enforce is 0 at boot and force_enforce is 1, won't LoadPin fail to
enforce pinning in loadpin_check() and loadpin_sb_free_security(), while
proc_handler_loadpin() simultaneously blocks userspace from enabling it via
/proc/sys/kernel/loadpin/enforce, locking LoadPin in non-enforcing mode
until reboot (and making force_enforce a complete no-op when
CONFIG_SYSCTL=n)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023171508.3460300-1-acrate@waldn.net?part=1