| 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: <1aaaf30d-f727-63c4-e5ee-e88ff51af300@digikod.net>
Date: Mon, 16 May 2022 20:28:01 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: willemdebruijn.kernel@...il.com,
linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org, yusongping@...wei.com,
anton.sirazetdinov@...wei.com
Subject: Re: [PATCH v5 04/15] landlock: helper functions refactoring
On 16/05/2022 19:43, Konstantin Meskhidze wrote:
>
>
> 5/16/2022 8:14 PM, Mickaël Salaün пишет:
>>
>> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>>> Unmask_layers(), init_layer_masks() and
>>> get_handled_accesses() helper functions move to
>>> ruleset.c and rule_type argument is added.
>>> This modification supports implementing new rule
>>> types into next landlock versions.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
[...]
>>> -/*
>>> - * @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])
>>
>> Moving these entire blocks of code make the review/diff impossible.
>> Why moving these helpers?
>
> Cause these helpers are going to be used both for filesystem and
> network. I moved them into ruleset.c/h
Right. Please create a commit which only moves these helpers without
modifying them, and explain in the commit message that this removes
inlined code. We'll see later if this adds a visible performance impact.
[...]
>>> @@ -519,17 +413,25 @@ static int check_access_path_dual(
>>>
>>> if (unlikely(dentry_child1)) {
>>> unmask_layers(find_rule(domain, dentry_child1),
>>> - init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - &_layer_masks_child1),
>>> - &_layer_masks_child1);
>>> + init_layer_masks(domain,
>>> + LANDLOCK_MASK_ACCESS_FS,
>>> + &_layer_masks_child1,
>>> + sizeof(_layer_masks_child1),
>>> + LANDLOCK_RULE_PATH_BENEATH),
>>> + &_layer_masks_child1,
>>> + ARRAY_SIZE(_layer_masks_child1));
>>
>> There is a lot of formatting diff and that makes the review difficult.
>> Please format everything with clang-format-14.
>
> Ok. Do you have some tool that helps you with editing code with clang
> format?
I just run `clang-format-14 -i` on files before each commit. Some
editors such as VSCode can handle the clang-format configuration (which
is in the Linux source tree).
>>
>>> layer_masks_child1 = &_layer_masks_child1;
>>> child1_is_directory = d_is_dir(dentry_child1);
>>> }
>>> if (unlikely(dentry_child2)) {
>>> unmask_layers(find_rule(domain, dentry_child2),
>>> - init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - &_layer_masks_child2),
>>> - &_layer_masks_child2);
>>> + init_layer_masks(domain,
>>> + LANDLOCK_MASK_ACCESS_FS,
>>> + &_layer_masks_child2,
>>> + sizeof(_layer_masks_child2),
>>> + LANDLOCK_RULE_PATH_BENEATH),
>>> + &_layer_masks_child2,
>>> + ARRAY_SIZE(_layer_masks_child2));
>>> layer_masks_child2 = &_layer_masks_child2;
>>> child2_is_directory = d_is_dir(dentry_child2);
>>> }
>>> @@ -582,14 +484,15 @@ static int check_access_path_dual(
>>>
>>> rule = find_rule(domain, walker_path.dentry);
>>> allowed_parent1 = unmask_layers(rule, access_masked_parent1,
>>> - layer_masks_parent1);
>>> + layer_masks_parent1,
>>> + ARRAY_SIZE(*layer_masks_parent1));
>>> allowed_parent2 = unmask_layers(rule, access_masked_parent2,
>>> - layer_masks_parent2);
>>> + layer_masks_parent2,
>>> + ARRAY_SIZE(*layer_masks_parent2));
>>>
>>> /* Stops when a rule from each layer grants access. */
>>> if (allowed_parent1 && allowed_parent2)
>>> break;
>>> -
>>
>> There is no place for such formatting/whitespace patches.
>>
> I missed that. scripts/checkpatch.pl did not show any problem here.
checkpatch.pl doesn't warn about whitespace changes.
> I will fix it. Thanks.
>>
>>> jump_up:
>>> if (walker_path.dentry == walker_path.mnt->mnt_root) {
>>> if (follow_up(&walker_path)) {
>>> @@ -645,7 +548,9 @@ static inline int check_access_path(const struct
>>> landlock_ruleset *const domain,
>>> {
>>> layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>>
>>> - access_request = init_layer_masks(domain, access_request,
>>> &layer_masks);
>>> + access_request = init_layer_masks(domain, access_request,
>>> + &layer_masks, sizeof(layer_masks),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>> return check_access_path_dual(domain, path, access_request,
>>> &layer_masks, NULL, 0, NULL, NULL);
>>> }
>>> @@ -729,7 +634,8 @@ static bool collect_domain_accesses(
>>> return true;
>>>
>>> access_dom = init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - layer_masks_dom);
>>> + layer_masks_dom, sizeof(*layer_masks_dom),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>>
>>> dget(dir);
>>> while (true) {
>>> @@ -737,7 +643,8 @@ static bool collect_domain_accesses(
>>>
>>> /* Gets all layers allowing all domain accesses. */
>>> if (unmask_layers(find_rule(domain, dir), access_dom,
>>> - layer_masks_dom)) {
>>> + layer_masks_dom,
>>> + ARRAY_SIZE(*layer_masks_dom))) {
>>> /*
>>> * Stops when all handled accesses are allowed by at
>>> * least one rule in each layer.
>>> @@ -851,9 +758,10 @@ static int current_check_refer_path(struct
>>> dentry *const old_dentry,
>>> * The LANDLOCK_ACCESS_FS_REFER access right is not required
>>> * for same-directory referer (i.e. no reparenting).
>>> */
>>> - access_request_parent1 = init_layer_masks(
>>> - dom, access_request_parent1 | access_request_parent2,
>>> - &layer_masks_parent1);
>>> + access_request_parent1 = init_layer_masks(dom,
>>> + access_request_parent1 | access_request_parent2,
>>> + &layer_masks_parent1, sizeof(layer_masks_parent1),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>> return check_access_path_dual(dom, new_dir,
>>> access_request_parent1,
>>> &layer_masks_parent1, NULL, 0,
>>> @@ -861,7 +769,9 @@ static int current_check_refer_path(struct dentry
>>> *const old_dentry,
>>> }
>>>
>>> /* Backward compatibility: no reparenting support. */
>>> - if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
>>> + if (!(get_handled_accesses(dom, LANDLOCK_RULE_PATH_BENEATH,
>>> + LANDLOCK_NUM_ACCESS_FS) &
>>> + LANDLOCK_ACCESS_FS_REFER))
>>> return -EXDEV;
>>>
>>> access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index 4b4c9953bb32..c4ed783d655b 100644
>>> --- a/security/landlock/ruleset.c
>>> +++ b/security/landlock/ruleset.c
>>> @@ -233,7 +233,8 @@ static int insert_rule(struct landlock_ruleset
>>> *const ruleset,
>>> &(*layers)[0]);
>>> if (IS_ERR(new_rule))
>>> return PTR_ERR(new_rule);
>>> - rb_replace_node(&this->node, &new_rule->node,
>>> &ruleset->root_inode);
>>> + rb_replace_node(&this->node, &new_rule->node,
>>> + &ruleset->root_inode);
>>
>> This is a pure formatting hunk. :/
>>
> Thats strange, cause in my editor I have normal aligment of arguments.
> Could please share clang-format-14 tab size and other format
> parameters?
They are in the .clang-format file. It would be much easier to just run
clang-format-14 -i on your changed files. I guess you had different
changes between consecutive commits.
Powered by blists - more mailing lists