On 29.12.23 23:31, Paul Moore wrote:
> On Wed, Dec 27, 2023 at 9:31 AM Michael Weiß
> <michael.weiss@xxxxxxxxxxxxxxxxxxx> wrote:
>> 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.
>
> I think it's worth taking a minute to talk about this proposed change
> and the existing security_task_prctl() hook, as there is an important
> difference between the two which is the source of my concern.
>
> If you look at the prctl() syscall implementation, right at the top of
> the function you see the LSM hook:
>
> SYSCALL_DEFINE(prctl, ...)
> {
> ...
>
> error = security_task_prctl(...);
> if (error != -ENOSYS)
> return error;
>
> error = 0;
>
> ....
> }
>
> While it is true that the LSM hook returns a "special" value, -ENOSYS,
> from a practical perspective this is not significantly different from
> the much more common zero value used to indicate no restriction from
> the LSM layer. However, the more important thing to note is that the
> return value from security_task_prctl() does not influence any other
> access controls in the caller outside of those implemented inside the
> LSM; in fact the error code is reset to zero immediately after the LSM
> hook.
>
> More on this below ...
>
>> 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;
>
> This is my concern, depending on what the LSM hook returns, the
> superblock's flags are set differently, affecting much more than just
> a LSM-based security mechanism.
>
> LSMs should not be able to undermine, shortcut, or otherwise bypass
> access controls built into other parts of the kernel. In other words,
> a LSM should only ever be able to deny an operation, it should not be
> able to permit an operation that otherwise would have been denied.
Hmm, OK. Then I can't see to come here any further as we would directly
or indirectly set the superblock flags based on if a security hook is
implemented or not, which I understand now is against LSM architecture.
Thanks Paul for clarification.
Christian, what do you think?
Maybe we just set the SB_I_NODEV and SB_I_MANGED_DEVICES flag based on
a sysctl at the same place for backward compatibility,
drop the additional security hook and keep the rest as is from your
proposal:
if (sysctl_managed_devices)
s->s_iflags |= SB_I_MANAGED_DEVICES;
else if (s->s_user_ns != &init_user_ns)
s->s_iflags |= SB_I_NODEV;
A device access managing LSM can then just implement the current
security_sb_alloc() hook to deny creating the super block at all.
>
>> INIT_HLIST_NODE(&s->s_instances);
>> INIT_HLIST_BL_HEAD(&s->s_roots);
>> mutex_init(&s->s_sync_lock);
>
