Re: [RFC PATCH v3 3/3] devguard: added device guard for mknod in non-initial userns

[Casey Schaufler <casey@schaufler-ca.com>]

On 12/13/2023 6:38 AM, Michael Weiß wrote:
> devguard is a simple LSM to allow CAP_MKNOD in non-initial user
> namespace in cooperation of an attached cgroup device program. We
> just need to implement the security_inode_mknod() hook for this.
> In the hook, we check if the current task is guarded by a device
> cgroup using the lately introduced cgroup_bpf_current_enabled()
> helper. If so, we strip out SB_I_NODEV from the super block.
>
> Access decisions to those device nodes are then guarded by existing
> device cgroups mechanism.
>
> Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
> ---
>  security/Kconfig             | 11 +++++----
>  security/Makefile            |  1 +
>  security/devguard/Kconfig    | 12 ++++++++++
>  security/devguard/Makefile   |  2 ++
>  security/devguard/devguard.c | 44 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 65 insertions(+), 5 deletions(-)
>  create mode 100644 security/devguard/Kconfig
>  create mode 100644 security/devguard/Makefile
>  create mode 100644 security/devguard/devguard.c
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 52c9af08ad35..7ec4017745d4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -194,6 +194,7 @@ source "security/yama/Kconfig"
>  source "security/safesetid/Kconfig"
>  source "security/lockdown/Kconfig"
>  source "security/landlock/Kconfig"
> +source "security/devguard/Kconfig"
>  
>  source "security/integrity/Kconfig"
>  
> @@ -233,11 +234,11 @@ endchoice
>  
>  config LSM
>  	string "Ordered list of enabled LSMs"
> -	default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> -	default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> -	default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> -	default "landlock,lockdown,yama,loadpin,safesetid,bpf" if DEFAULT_SECURITY_DAC
> -	default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf"
> +	default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf,devguard" if DEFAULT_SECURITY_SMACK
> +	default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf,devguard" if DEFAULT_SECURITY_APPARMOR
> +	default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf,devguard" if DEFAULT_SECURITY_TOMOYO
> +	default "landlock,lockdown,yama,loadpin,safesetid,bpf,devguard" if DEFAULT_SECURITY_DAC
> +	default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf,devguard"
>  	help
>  	  A comma-separated list of LSMs, in initialization order.
>  	  Any LSMs left off this list, except for those with order
> diff --git a/security/Makefile b/security/Makefile
> index 18121f8f85cd..82a0d8cab3c3 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
>  obj-$(CONFIG_CGROUPS)			+= device_cgroup.o
>  obj-$(CONFIG_BPF_LSM)			+= bpf/
>  obj-$(CONFIG_SECURITY_LANDLOCK)		+= landlock/
> +obj-$(CONFIG_SECURITY_DEVGUARD)		+= devguard/
>  
>  # Object integrity file lists
>  obj-$(CONFIG_INTEGRITY)			+= integrity/
> diff --git a/security/devguard/Kconfig b/security/devguard/Kconfig
> new file mode 100644
> index 000000000000..592684615a8f
> --- /dev/null
> +++ b/security/devguard/Kconfig
> @@ -0,0 +1,12 @@
> +config SECURITY_DEVGUARD
> +	bool "Devguard for device node creation"
> +	depends on SECURITY
> +	depends on CGROUP_BPF
> +	default n
> +	help
> +	  This enables devguard, an LSM that allows to guard device node
> +	  creation in non-initial user namespace. It may allow mknod
> +	  in cooperation of an attached cgroup device program.
> +	  This security module stacks with other LSMs.
> +
> +	  If you are unsure how to answer this question, answer N.
> diff --git a/security/devguard/Makefile b/security/devguard/Makefile
> new file mode 100644
> index 000000000000..fdaff8dc2fea
> --- /dev/null
> +++ b/security/devguard/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_SECURITY_DEVGUARD) += devguard.o
> diff --git a/security/devguard/devguard.c b/security/devguard/devguard.c
> new file mode 100644
> index 000000000000..3a0c9c27a691
> --- /dev/null
> +++ b/security/devguard/devguard.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device guard security module
> + *
> + * Simple in-kernel LSM to allow cap_mknod in non-initial
> + * user namespace if current task is guarded by device cgroup.
> + *
> + * Copyright (C) 2023 Fraunhofer AISEC. All rights reserved.
> + *
> + * Authors: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
> + */
> +
> +#include <linux/bpf-cgroup.h>
> +#include <linux/lsm_hooks.h>
> +
> +static int devguard_inode_mknod(struct inode *dir, struct dentry *dentry,
> +				umode_t mode, dev_t dev)
> +{
> +	if (dentry->d_sb->s_iflags & ~SB_I_NODEV)
> +		return 0;
> +
> +	// strip SB_I_NODEV on super block if device cgroup is active

Please use block style comments. We don't use // comments here.

	/*
	 * Strip SB_I_NODEV on super block if device cgroup is active
	 */

> +	if (cgroup_bpf_current_enabled(CGROUP_DEVICE))
> +		dentry->d_sb->s_iflags &= ~SB_I_NODEV;
> +
> +	return 0;
> +}
> +
> +static struct security_hook_list devguard_hooks[] __ro_after_init = {
> +	LSM_HOOK_INIT(inode_mknod, devguard_inode_mknod),
> +};
> +
> +static int __init devguard_init(void)
> +{
> +	security_add_hooks(devguard_hooks, ARRAY_SIZE(devguard_hooks),
> +			   "devguard");
> +	pr_info("devguard: initialized\n");
> +	return 0;
> +}
> +
> +DEFINE_LSM(devguard) = {
> +	.name = "devguard",
> +	.init = devguard_init,
> +};