| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <edcbbdd9-ea0e-5a65-1be2-3c4ab8ee251c@huawei.com>
Date: Fri, 18 Mar 2022 14:36:06 +0300
From: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>,
<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
<artem.kuzin@...wei.com>, <anton.sirazetdinov@...wei.com>
Subject: Re: [RFC PATCH v4 02/15] landlock: filesystem access mask helpers
3/17/2022 9:03 PM, Mickaël Salaün пишет:
>
> On 17/03/2022 14:25, Konstantin Meskhidze wrote:
>>
>>
>> 3/15/2022 8:48 PM, Mickaël Salaün пишет:
>
> …
>
>>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>>> index 78341a0538de..a6212b752549 100644
>>>> --- a/security/landlock/ruleset.c
>>>> +++ b/security/landlock/ruleset.c
>>>> @@ -44,16 +44,30 @@ static struct landlock_ruleset
>>>> *create_ruleset(const u32 num_layers)
>>>> return new_ruleset;
>>>> }
>>>>
>>>> -struct landlock_ruleset *landlock_create_ruleset(const u32
>>>> access_mask)
>>>> +/* A helper function to set a filesystem mask */
>>>> +void landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
>>>
>>> struct landlock_ruleset *const ruleset
>>>
>>> Please use const as much as possible even in function arguments: e.g.
>>> access_masks_set, mask_level…
>>>
>>>> + const struct landlock_access_mask *access_mask_set,
>>
>> Ok. Got it.
>>>
>>> nit: no need for "_set" suffix.
>>
>> Ok. Thanks
>>>
>>> Why do you need a struct landlock_access_mask and not just u16 (which
>>> will probably become a subset of access_mask_t, see [1])?
>>> landlock_create_ruleset() could just take two masks as argument instead.
>>>
>>> [1] https://lore.kernel.org/all/20220221212522.320243-2-mic@digikod.net/
>>
>> This was your suggestion in previous patch V3:
>>
>> " To make it easier and avoid mistakes, you could use a dedicated
>> struct to properly manage masks passing and conversions:
>> struct landlock_access_mask {
>> u16 fs; // TODO: make sure at build-time that all access rights
>> fit in.
>> u16 net; // TODO: ditto for network access rights.
>> }
>>
>> get_access_masks(const struct landlock_ruleset *, struct
>> landlock_access_mask *);
>> set_access_masks(struct landlock_ruleset *, const struct
>> landlock_access_mask *);
>>
>> This should also be part of a standalone patch."
>>
>>
>> https://lore.kernel.org/linux-security-module/ed2bd420-a22b-2912-1ff5-f48ab352d8e7@digikod.net/
>
>
> Indeed! What is nice about struct is that it enables to easily
> differentiate same-type values (e.g. fs mask from net mask). However,
> because this struct is mainly passed once to initialize a ruleset, it
> looks like this was not worth it. Please get back to how you dealt with
> that previously but with a new access_mask_t typedef, which will
> conflict with my latest patchset but that will be trivial to fix. You
> can also merge the landlock_set_*_access_mask() into
> landlock_create_ruleset() because they are not use elsewhere (and then
> it would have been much less useful to have a dedicated struct).
I got your point here. Thanks. I will get back to the previous way.
>
>
>>
>>
>>>
>>>> + u16 mask_level)
>>>> +{
>>>> + ruleset->access_masks[mask_level] = access_mask_set->fs;
>>>> +}
>>>> +
>>>> +/* A helper function to get a filesystem mask */
>>>> +u32 landlock_get_fs_access_mask(const struct landlock_ruleset
>>>> *ruleset, u16 mask_level)
>>>> +{
>>>> + return ruleset->access_masks[mask_level];
>>>> +}
>>>
>>> You can move these two helpers to ruleset.h and make them static inline.
>>
>> Ok. I got it.
>>>
>>>> +
>>>> +struct landlock_ruleset *landlock_create_ruleset(const struct
>>>> landlock_access_mask *access_mask_set)
>>>> {
>>>> struct landlock_ruleset *new_ruleset;
>>>>
>>>> /* Informs about useless ruleset. */
>>>> - if (!access_mask)
>>>> + if (!access_mask_set->fs)
>>>> return ERR_PTR(-ENOMSG);
>>>> new_ruleset = create_ruleset(1);
>>>> if (!IS_ERR(new_ruleset))
>>>> - new_ruleset->access_masks[0] = access_mask;
>>>> + landlock_set_fs_access_mask(new_ruleset, access_mask_set, 0);
>>>> return new_ruleset;
>>>> }
>>>>
>>>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>>>> index 32d90ce72428..bc87e5f787f7 100644
>>>> --- a/security/landlock/ruleset.h
>>>> +++ b/security/landlock/ruleset.h
>>>> @@ -16,6 +16,16 @@
>>>>
>>>> #include "object.h"
>>>>
>>>> +/**
>>>> + * struct landlock_access_mask - A helper structure to handle
>>>> different mask types
>>>> + */
>>>> +struct landlock_access_mask {
>>>> + /**
>>>> + * @fs: Filesystem access mask.
>>>> + */
>>>> + u16 fs;
>>>> +};
>>>
>>> Removing this struct would simplify the code.
>>
>> I followed your recommendation to use such kind of structure.
>> Please check previous patch V3 review:
>>
>>
>> https://lore.kernel.org/linux-security-module/ed2bd420-a22b-2912-1ff5-f48ab352d8e7@digikod.net/
>>
>>
>>>
>>>> +
>>>> /**
>>>> * struct landlock_layer - Access rights for a given layer
>>>> */
>>>> @@ -140,7 +150,8 @@ struct landlock_ruleset {
>>>> };
>>>> };
>>>>
>>>> -struct landlock_ruleset *landlock_create_ruleset(const u32
>>>> access_mask);
>>>> +struct landlock_ruleset *landlock_create_ruleset(const struct
>>>> landlock_access_mask
>>>> + *access_mask_set);
>>>>
>>>> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>>>> void landlock_put_ruleset_deferred(struct landlock_ruleset *const
>>>> ruleset);
>>>> @@ -162,4 +173,10 @@ static inline void landlock_get_ruleset(struct
>>>> landlock_ruleset *const ruleset)
>>>> refcount_inc(&ruleset->usage);
>>>> }
>>>>
>>>> +void landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
>>>> + const struct landlock_access_mask *access_mask_set,
>>>> + u16 mask_level);
>>>> +
>>>> +u32 landlock_get_fs_access_mask(const struct landlock_ruleset
>>>> *ruleset, u16 mask_level);
>>>> +
>>>> #endif /* _SECURITY_LANDLOCK_RULESET_H */
>>>> diff --git a/security/landlock/syscalls.c
>>>> b/security/landlock/syscalls.c
>>>> index f1d86311df7e..5931b666321d 100644
>>>> --- a/security/landlock/syscalls.c
>>>> +++ b/security/landlock/syscalls.c
>>>> @@ -159,6 +159,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>>> {
>>>> struct landlock_ruleset_attr ruleset_attr;
>>>> struct landlock_ruleset *ruleset;
>>>> + struct landlock_access_mask access_mask_set = {.fs = 0};
>>>> int err, ruleset_fd;
>>>>
>>>> /* Build-time checks. */
>>>> @@ -185,9 +186,10 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>>> if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
>>>> LANDLOCK_MASK_ACCESS_FS)
>>>> return -EINVAL;
>>>> + access_mask_set.fs = ruleset_attr.handled_access_fs;
>>>>
>>>> /* Checks arguments and transforms to kernel struct. */
>>>> - ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs);
>>>> + ruleset = landlock_create_ruleset(&access_mask_set);
>>>> if (IS_ERR(ruleset))
>>>> return PTR_ERR(ruleset);
>>>>
>>>> @@ -343,8 +345,9 @@ SYSCALL_DEFINE4(landlock_add_rule,
>>>> * Checks that allowed_access matches the @ruleset constraints
>>>> * (ruleset->access_masks[0] is automatically upgraded to
>>>> 64-bits).
>>>> */
>>>> - if ((path_beneath_attr.allowed_access |
>>>> ruleset->access_masks[0]) !=
>>>> - ruleset->access_masks[0]) {
>>>> +
>>>> + if ((path_beneath_attr.allowed_access |
>>>> landlock_get_fs_access_mask(ruleset, 0)) !=
>>>> + landlock_get_fs_access_mask(ruleset, 0)) {
>>>> err = -EINVAL;
>>>> goto out_put_ruleset;
>>>> }
>>>> --
>>>> 2.25.1
>>>>
>>> .
> .
Powered by blists - more mailing lists