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

[Michael Weiß]

On 23.12.23 00:39, Paul Moore wrote:
> On Mon, Dec 18, 2023 at 7:30 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>> I'm not generally opposed to kfuncs ofc but here it just seems a bit
>> pointless. What we want is to keep SB_I_{NODEV,MANAGED_DEVICES} confined
>> to alloc_super(). The only central place it's raised where we control
>> all locking and logic. So it doesn't even have to appear in any
>> security_*() hooks.
>>
>> diff --git a/security/security.c b/security/security.c
>> index 088a79c35c26..bf440d15615d 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1221,6 +1221,33 @@ int security_sb_alloc(struct super_block *sb)
>>         return rc;
>>  }
>>
>> +/*
>> + * security_sb_device_access() - Let LSMs handle device access
>> + * @sb: filesystem superblock
>> + *
>> + * Let an LSM take over device access management for this superblock.
>> + *
>> + * Return: Returns 1 if LSMs handle device access, 0 if none does and -ERRNO on
>> + *         failure.
>> + */
>> +int security_sb_device_access(struct super_block *sb)
>> +{
>> +       int thisrc;
>> +       int rc = LSM_RET_DEFAULT(sb_device_access);
>> +       struct security_hook_list *hp;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.sb_device_access, list) {
>> +               thisrc = hp->hook.sb_device_access(sb);
>> +               if (thisrc < 0)
>> +                       return thisrc;
>> +               /* At least one LSM claimed device access management. */
>> +               if (thisrc == 1)
>> +                       rc = 1;
>> +       }
>> +
>> +       return rc;
>> +}
> 
> I worry that this hook, and the way it is plumbed into alloc_super()
> below, brings us back to the problem of authoritative LSM hooks which
> is something I can't support at this point in time.  The same can be
> said for a LSM directly flipping bits in the superblock struct.
> 
> The LSM should not grant any additional privilege, either directly in
> the LSM code, or indirectly via the caller; the LSM should only
> restrict operations which would have otherwise been allowed.
> 
> The LSM should also refrain from modifying any kernel data structures
> that do not belong directly to the LSM.  A LSM caller may modify
> kernel data structures that it owns based on the result of the LSM
> hook, so long as those modifications do not grant additional privilege
> as described above.

Hi Paul, what would you think about if we do it as shown in the
patch below (untested)?

I have adapted Christians patch slightly in a way that we do let
all LSMs agree on if device access management should be done or not.
Similar to the security_task_prctl() hook.

The new default behavior in alloc_super() is that the
SB_I_MANANGED_DEVICES flag is set if no error is returned by the
security hook, otherwise the old semantic which raises SB_I_NODEV
is used if an LSM does not agree on device management for the
superblock.

So a LSM can only be used to restrict access to the superblock
but not to do more privileged operations, since the
MANAGED_DEVICES would be allowed by default.

The LSM default value is set to -EOPNOSUPP to decide if an LSM has
implemented the hook inside of fs/super.c for backward compatibility.

> 
>> diff --git a/fs/super.c b/fs/super.c
>> index 076392396e72..2295c0f76e56 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -325,7 +325,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>>  {
>>         struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);
>>         static const struct super_operations default_op;
>> -       int i;
>> +       int err, i;
>>
>>         if (!s)
>>                 return NULL;
>> @@ -362,8 +362,16 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>>         }
>>         s->s_bdi = &noop_backing_dev_info;
>>         s->s_flags = flags;
>> -       if (s->s_user_ns != &init_user_ns)
>> +
>> +       err = security_sb_device_access(s);
>> +       if (err < 0)
>> +               goto fail;
>> +
>> +       if (err)
>> +               s->s_iflags |= SB_I_MANAGED_DEVICES;
>> +       else if (s->s_user_ns != &init_user_ns)
>>                 s->s_iflags |= SB_I_NODEV;
>> +
>>         INIT_HLIST_NODE(&s->s_instances);
>>         INIT_HLIST_BL_HEAD(&s->s_roots);
>>         mutex_init(&s->s_sync_lock);
> 
---
 fs/namespace.c                | 11 +++++++----
 fs/super.c                    | 12 ++++++++++--
 include/linux/fs.h            |  1 +
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  6 ++++++
 security/security.c           | 26 ++++++++++++++++++++++++++
 6 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index fbf0e596fcd3..e87cc0320091 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4887,7 +4887,6 @@ static bool mnt_already_visible(struct mnt_namespace *ns,
 
 static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags)
 {
-	const unsigned long required_iflags = SB_I_NOEXEC | SB_I_NODEV;
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	unsigned long s_iflags;
 
@@ -4899,9 +4898,13 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
 	if (!(s_iflags & SB_I_USERNS_VISIBLE))
 		return false;
 
-	if ((s_iflags & required_iflags) != required_iflags) {
-		WARN_ONCE(1, "Expected s_iflags to contain 0x%lx\n",
-			  required_iflags);
+	if (!(s_iflags & SB_I_NOEXEC)) {
+		WARN_ONCE(1, "Expected s_iflags to contain SB_I_NOEXEC\n");
+		return true;
+	}
+
+	if (!(s_iflags & (SB_I_NODEV | SB_I_MANAGED_DEVICES))) {
+		WARN_ONCE(1, "Expected s_iflags to contain device access mask\n");
 		return true;
 	}
 
diff --git a/fs/super.c b/fs/super.c
index 076392396e72..6510168d51ce 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -325,7 +325,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 {
 	struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);
 	static const struct super_operations default_op;
-	int i;
+	int i, err;
 
 	if (!s)
 		return NULL;
@@ -362,8 +362,16 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	}
 	s->s_bdi = &noop_backing_dev_info;
 	s->s_flags = flags;
-	if (s->s_user_ns != &init_user_ns)
+
+	err = security_sb_device_access(s);
+	if (err < 0 && err != -EOPNOTSUPP)
+		goto fail;
+
+	if (err && s->s_user_ns != &init_user_ns)
 		s->s_iflags |= SB_I_NODEV;
+	else
+		s->s_iflags |= SB_I_MANAGED_DEVICES;
+
 	INIT_HLIST_NODE(&s->s_instances);
 	INIT_HLIST_BL_HEAD(&s->s_roots);
 	mutex_init(&s->s_sync_lock);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..6ca0fe922478 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1164,6 +1164,7 @@ extern int send_sigurg(struct fown_struct *fown);
 #define SB_I_USERNS_VISIBLE		0x00000010 /* fstype already mounted */
 #define SB_I_IMA_UNVERIFIABLE_SIGNATURE	0x00000020
 #define SB_I_UNTRUSTED_MOUNTER		0x00000040
+#define SB_I_MANAGED_DEVICES		0x00000080
 
 #define SB_I_SKIP_SYNC	0x00000100	/* Skip superblock at global sync */
 #define SB_I_PERSB_BDI	0x00000200	/* has a per-sb bdi */
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ff217a5ce552..1dc13b7e9a4a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -60,6 +60,7 @@ LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc,
 LSM_HOOK(int, -ENOPARAM, fs_context_parse_param, struct fs_context *fc,
 	 struct fs_parameter *param)
 LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
+LSM_HOOK(int, -EOPNOTSUPP, sb_device_access, struct super_block *sb)
 LSM_HOOK(void, LSM_RET_VOID, sb_delete, struct super_block *sb)
 LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
 LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1d1df326c881..79f2b201c7bd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -298,6 +298,7 @@ int security_fs_context_submount(struct fs_context *fc, struct super_block *refe
 int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc);
 int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param);
 int security_sb_alloc(struct super_block *sb);
+int security_sb_device_access(struct super_block *sb);
 void security_sb_delete(struct super_block *sb);
 void security_sb_free(struct super_block *sb);
 void security_free_mnt_opts(void **mnt_opts);
@@ -652,6 +653,11 @@ static inline int security_sb_alloc(struct super_block *sb)
 	return 0;
 }
 
+static inline int security_sb_device_access(struct super_block *sb)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void security_sb_delete(struct super_block *sb)
 { }
 
diff --git a/security/security.c b/security/security.c
index dcb3e7014f9b..054ee19edef0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1221,6 +1221,32 @@ int security_sb_alloc(struct super_block *sb)
 	return rc;
 }
 
+/*
+ * security_sb_device_access() - handle device access on this sb
+ * @sb: filesystem superblock
+ *
+ * Let LSMs decide if device access management is allowed for this superblock.
+ *
+ * Return: Returns 0 if LSMs handle device access, -EOPNOTSUPP if none does and
+ * 	   -ERRNO on any other failure.
+ */
+int security_sb_device_access(struct super_block *sb)
+{
+	int thisrc;
+	int rc = LSM_RET_DEFAULT(sb_device_access);
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.sb_device_access, list) {
+		thisrc = hp->hook.sb_device_access(sb);
+		if (thisrc != LSM_RET_DEFAULT(sb_device_access)) {
+			rc = thisrc;
+			if (thisrc != 0)
+				break;
+		}
+	}
+	return rc;
+}
+
 /**
  * security_sb_delete() - Release super_block LSM associated objects
  * @sb: filesystem superblock
--