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
--
