Re: [RFC PATCH v4 02/15] landlock: filesystem access mask helpers

[Konstantin Meskhidze <konstantin.meskhidze@huawei.com>]

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