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.
> 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);
--
paul-moore.com
