[PATCH v8 04/12] landlock: Move unmask_layers() and init_layer_masks()
Konstantin Meskhidze (A)
konstantin.meskhidze at huawei.com
Mon Nov 28 03:25:15 UTC 2022
11/17/2022 9:42 PM, Mickaël Salaün пишет:
>
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This patch moves unmask_layers() and init_layer_masks() helpers
>> to ruleset.c to share with landlock network implementation in
>
> …to share them with the Landlock network implementation in
>
Got it.
>
>> following commits.
>>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
>> ---
>>
>> Changes since v7:
>> * Refactors commit message.
>>
>> Changes since v6:
>> * Moves get_handled_accesses() helper from ruleset.c back to fs.c,
>> cause it's not used in coming network commits.
>>
>> Changes since v5:
>> * Splits commit.
>> * Moves init_layer_masks() and get_handled_accesses() helpers
>> to ruleset.c and makes then non-static.
>> * Formats code with clang-format-14.
>>
>> ---
>> security/landlock/fs.c | 103 ------------------------------------
>> security/landlock/ruleset.c | 102 +++++++++++++++++++++++++++++++++++
>> security/landlock/ruleset.h | 20 +++++++
>> 3 files changed, 122 insertions(+), 103 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 710cfa1306de..240e42a8f788 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -226,60 +226,6 @@ find_rule(const struct landlock_ruleset *const domain,
>> return rule;
>> }
>>
>> -/*
>> - * @layer_masks is read and may be updated according to the access request and
>> - * the matching rule.
>> - *
>> - * Returns true if the request is allowed (i.e. relevant layer masks for the
>> - * request are empty).
>> - */
>> -static inline bool
>> -unmask_layers(const struct landlock_rule *const rule,
>> - const access_mask_t access_request,
>> - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
>> -{
>> - size_t layer_level;
>> -
>> - if (!access_request || !layer_masks)
>> - return true;
>> - if (!rule)
>> - return false;
>> -
>> - /*
>> - * An access is granted if, for each policy layer, at least one rule
>> - * encountered on the pathwalk grants the requested access,
>> - * regardless of its position in the layer stack. We must then check
>> - * the remaining layers for each inode, from the first added layer to
>> - * the last one. When there is multiple requested accesses, for each
>> - * policy layer, the full set of requested accesses may not be granted
>> - * by only one rule, but by the union (binary OR) of multiple rules.
>> - * E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
>> - */
>> - for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
>> - const struct landlock_layer *const layer =
>> - &rule->layers[layer_level];
>> - const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
>> - const unsigned long access_req = access_request;
>> - unsigned long access_bit;
>> - bool is_empty;
>> -
>> - /*
>> - * Records in @layer_masks which layer grants access to each
>> - * requested access.
>> - */
>> - is_empty = true;
>> - for_each_set_bit(access_bit, &access_req,
>> - ARRAY_SIZE(*layer_masks)) {
>> - if (layer->access & BIT_ULL(access_bit))
>> - (*layer_masks)[access_bit] &= ~layer_bit;
>> - is_empty = is_empty && !(*layer_masks)[access_bit];
>> - }
>> - if (is_empty)
>> - return true;
>> - }
>> - return false;
>> -}
>> -
>> /*
>> * Allows access to pseudo filesystems that will never be mountable (e.g.
>> * sockfs, pipefs), but can still be reachable through
>> @@ -303,55 +249,6 @@ get_handled_accesses(const struct landlock_ruleset *const domain)
>> return access_dom & LANDLOCK_MASK_ACCESS_FS;
>> }
>>
>> -/**
>> - * init_layer_masks - Initialize layer masks from an access request
>> - *
>> - * Populates @layer_masks such that for each access right in @access_request,
>> - * the bits for all the layers are set where this access right is handled.
>> - *
>> - * @domain: The domain that defines the current restrictions.
>> - * @access_request: The requested access rights to check.
>> - * @layer_masks: The layer masks to populate.
>> - *
>> - * Returns: An access mask where each access right bit is set which is handled
>> - * in any of the active layers in @domain.
>> - */
>> -static inline access_mask_t
>> -init_layer_masks(const struct landlock_ruleset *const domain,
>> - const access_mask_t access_request,
>> - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
>> -{
>> - access_mask_t handled_accesses = 0;
>> - size_t layer_level;
>> -
>> - memset(layer_masks, 0, sizeof(*layer_masks));
>> - /* An empty access request can happen because of O_WRONLY | O_RDWR. */
>> - if (!access_request)
>> - return 0;
>> -
>> - /* Saves all handled accesses per layer. */
>> - for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
>> - const unsigned long access_req = access_request;
>> - unsigned long access_bit;
>> -
>> - for_each_set_bit(access_bit, &access_req,
>> - ARRAY_SIZE(*layer_masks)) {
>> - /*
>> - * Artificially handles all initially denied by default
>> - * access rights.
>> - */
>> - if (BIT_ULL(access_bit) &
>> - (landlock_get_fs_access_mask(domain, layer_level) |
>> - ACCESS_INITIALLY_DENIED)) {
>> - (*layer_masks)[access_bit] |=
>> - BIT_ULL(layer_level);
>> - handled_accesses |= BIT_ULL(access_bit);
>> - }
>> - }
>> - }
>> - return handled_accesses;
>> -}
>> -
>> /*
>> * Check that a destination file hierarchy has more restrictions than a source
>> * file hierarchy. This is only used for link and rename actions.
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index 961ffe0c709e..02ab14439c43 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -572,3 +572,105 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,
>> }
>> return NULL;
>> }
>> +
>> +/*
>> + * @layer_masks is read and may be updated according to the access request and
>> + * the matching rule.
>> + *
>> + * Returns true if the request is allowed (i.e. relevant layer masks for the
>> + * request are empty).
>> + */
>> +bool unmask_layers(const struct landlock_rule *const rule,
>> + const access_mask_t access_request,
>> + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
>> +{
>> + size_t layer_level;
>> +
>> + if (!access_request || !layer_masks)
>> + return true;
>> + if (!rule)
>> + return false;
>> +
>> + /*
>> + * An access is granted if, for each policy layer, at least one rule
>> + * encountered on the pathwalk grants the requested access,
>> + * regardless of its position in the layer stack. We must then check
>> + * the remaining layers for each inode, from the first added layer to
>> + * the last one. When there is multiple requested accesses, for each
>> + * policy layer, the full set of requested accesses may not be granted
>> + * by only one rule, but by the union (binary OR) of multiple rules.
>> + * E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
>> + */
>> + for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
>> + const struct landlock_layer *const layer =
>> + &rule->layers[layer_level];
>> + const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
>> + const unsigned long access_req = access_request;
>> + unsigned long access_bit;
>> + bool is_empty;
>> +
>> + /*
>> + * Records in @layer_masks which layer grants access to each
>> + * requested access.
>> + */
>> + is_empty = true;
>> + for_each_set_bit(access_bit, &access_req,
>> + ARRAY_SIZE(*layer_masks)) {
>> + if (layer->access & BIT_ULL(access_bit))
>> + (*layer_masks)[access_bit] &= ~layer_bit;
>> + is_empty = is_empty && !(*layer_masks)[access_bit];
>> + }
>> + if (is_empty)
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +/**
>> + * init_layer_masks - Initialize layer masks from an access request
>> + *
>> + * Populates @layer_masks such that for each access right in @access_request,
>> + * the bits for all the layers are set where this access right is handled.
>> + *
>> + * @domain: The domain that defines the current restrictions.
>> + * @access_request: The requested access rights to check.
>> + * @layer_masks: The layer masks to populate.
>> + *
>> + * Returns: An access mask where each access right bit is set which is handled
>> + * in any of the active layers in @domain.
>> + */
>> +access_mask_t
>> +init_layer_masks(const struct landlock_ruleset *const domain,
>> + const access_mask_t access_request,
>> + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
>> +{
>> + access_mask_t handled_accesses = 0;
>> + size_t layer_level;
>> +
>> + memset(layer_masks, 0, sizeof(*layer_masks));
>> + /* An empty access request can happen because of O_WRONLY | O_RDWR. */
>> + if (!access_request)
>> + return 0;
>> +
>> + /* Saves all handled accesses per layer. */
>> + for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
>> + const unsigned long access_req = access_request;
>> + unsigned long access_bit;
>> +
>> + for_each_set_bit(access_bit, &access_req,
>> + ARRAY_SIZE(*layer_masks)) {
>> + /*
>> + * Artificially handles all initially denied by default
>> + * access rights.
>> + */
>> + if (BIT_ULL(access_bit) &
>> + (landlock_get_fs_access_mask(domain, layer_level) |
>> + ACCESS_INITIALLY_DENIED)) {
>
> This is a future bug in the a next commit when
> landlock_get_fs_access_mask() is changed to get the network access mask.
> My patch will fix this part but you'll need to do a new copy of this
> function.
Ok. Thanks.
>
>
>
>> + (*layer_masks)[access_bit] |=
>> + BIT_ULL(layer_level);
>> + handled_accesses |= BIT_ULL(access_bit);
>> + }
>> + }
>> + }
>> + return handled_accesses;
>> +}
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 608ab356bc3e..50baff4fcbb4 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -34,6 +34,16 @@ typedef u16 layer_mask_t;
>> /* Makes sure all layers can be checked. */
>> static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
>>
>> +/*
>> + * All access rights that are denied by default whether they are handled or not
>> + * by a ruleset/layer. This must be ORed with all ruleset->fs_access_masks[]
>> + * entries when we need to get the absolute handled access masks.
>> + */
>> +/* clang-format off */
>> +#define ACCESS_INITIALLY_DENIED ( \
>> + LANDLOCK_ACCESS_FS_REFER)
>> +/* clang-format on */
>
> This ACCESS_INITIALLY_DENIED definition must be moved, not copied. You
> can rename ACCESS_INITIALLY_DENIED to ACCESS_FS_INITIALLY_DENIED and
> move this hunk before the access_mask_t definition.
>
Yep. Will be fixed.
>
>> +
>> /**
>> * struct landlock_layer - Access rights for a given layer
>> */
>> @@ -246,4 +256,14 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
>> LANDLOCK_SHIFT_ACCESS_FS) &
>> LANDLOCK_MASK_ACCESS_FS;
>> }
>> +
>> +bool unmask_layers(const struct landlock_rule *const rule,
>
> All public Landlock helpers must be prefixed with "landlock_"
Do you mean ones which are shared between fs and net parts?
>
>> + const access_mask_t access_request,
>> + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>> +
>> +access_mask_t
>> +init_layer_masks(const struct landlock_ruleset *const domain,
>> + const access_mask_t access_request,
>> + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>
> There is a warning generated by checkpatch.pl about this line:
> WARNING: function definition argument 'layer_mask_t' should also have
> an identifier name
>
> I think this is a bug in checkpatch.pl
>
I got this warn, but cant get rid of it.
Also think its a bug in checkpatck.pl
> Any though Andy, Joe, Dwaipayan or Lukas?
>
>
>> +
>> #endif /* _SECURITY_LANDLOCK_RULESET_H */
>> --
>> 2.25.1
>>
> .
More information about the Linux-security-module-archive
mailing list