-
Notifications
You must be signed in to change notification settings - Fork 0
landlock: Add LANDLOCK_ADD_RULE_QUIET #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: landlock-quiet-flag-base
Are you sure you want to change the base?
Conversation
52a5b61 to
de15882
Compare
ee7bedc to
434a24b
Compare
af4c481 to
92db84c
Compare
fcbc967 to
de94a09
Compare
de94a09 to
135d51a
Compare
0f12d29 to
c0cd55c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * is there to hold a quiet flag | ||
| */ | ||
| if (!path_beneath_attr.allowed_access) | ||
| if (!flags && !path_beneath_attr.allowed_access) |
Copilot
AI
Oct 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is unclear. The condition !flags should specifically check for the absence of LANDLOCK_ADD_RULE_QUIET rather than the absence of any flags, for better maintainability and clarity.
| if (!flags && !path_beneath_attr.allowed_access) | |
| if (!(flags & LANDLOCK_ADD_RULE_QUIET) && !path_beneath_attr.allowed_access) |
| * if it is there to hold a quiet flag | ||
| */ | ||
| if (!net_port_attr.allowed_access) | ||
| if (!flags && !net_port_attr.allowed_access) |
Copilot
AI
Oct 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as with filesystem rules: the condition !flags should specifically check for the absence of LANDLOCK_ADD_RULE_QUIET rather than the absence of any flags.
| if (!flags && !net_port_attr.allowed_access) | |
| if (!(flags & LANDLOCK_ADD_RULE_QUIET) && !net_port_attr.allowed_access) |
c0cd55c to
ab6bfc6
Compare
d724c6f to
54f9baf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (quiet_supported && getenv(ENV_NET_QUIET_NAME)) { | ||
| if (populate_ruleset_net(ENV_NET_QUIET_NAME, ruleset_fd, 0, | ||
| LANDLOCK_ADD_RULE_QUIET)) { | ||
| goto err_close_ruleset; | ||
| } | ||
| } |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent brace style. The preceding if blocks at lines 596-599 don't use braces for single-statement blocks, but this block at lines 618-623 does use braces. For consistency and per kernel coding style, either all or none of these similar if blocks should use braces. Consider applying the same style throughout.
| if (quiet_supported && getenv(ENV_NET_QUIET_NAME)) { | |
| if (populate_ruleset_net(ENV_NET_QUIET_NAME, ruleset_fd, 0, | |
| LANDLOCK_ADD_RULE_QUIET)) { | |
| goto err_close_ruleset; | |
| } | |
| } | |
| if (quiet_supported && getenv(ENV_NET_QUIET_NAME)) | |
| if (populate_ruleset_net(ENV_NET_QUIET_NAME, ruleset_fd, 0, | |
| LANDLOCK_ADD_RULE_QUIET)) | |
| goto err_close_ruleset; |
| access_mask_t missing = 0; | ||
| long youngest_layer = -1; | ||
|
|
||
| for_each_set_bit(access_bit, &access_req, layer_masks_size) { |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The type of variable mask changed from access_mask_t to layer_mask_t, which appears to be intentional for type correctness. However, this is a semantic change in an otherwise unrelated patch. Consider whether this fix should be in a separate commit or add a comment explaining why this type change is needed.
| for_each_set_bit(access_bit, &access_req, layer_masks_size) { | |
| for_each_set_bit(access_bit, &access_req, layer_masks_size) { | |
| /* | |
| * Use layer_mask_t for mask for type correctness, as | |
| * (*layer_masks)[access_bit] returns a layer_mask_t. | |
| */ |
ba86be5 to
c9ed524
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * are ignored in path walks. However, the rule is not useless if it | ||
| * is there to hold a quiet flag |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] In the error handling comment on lines 354-355, "However, the rule is not useless if it is there to hold a quiet flag" - this should probably say "However, the rule is not useless if the LANDLOCK_ADD_RULE_QUIET flag is set" to be more precise. The current wording "there to hold a quiet flag" is unclear about what "hold" means in this context.
| * are ignored in path walks. However, the rule is not useless if it | |
| * is there to hold a quiet flag | |
| * are ignored in path walks. However, the rule is not useless if the | |
| * LANDLOCK_ADD_RULE_QUIET flag is set. |
| * @level: Position of this layer in the layer stack. Starts from 1. | ||
| */ | ||
| u16 level; | ||
| u8 level; |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The struct landlock_layer uses u8 level but this limits support to 256 layers. However, LANDLOCK_MAX_NUM_LAYERS is defined as 16 (in common.h). The previous u16 level could support up to 65536 layers, but this change reduces it to 256. While 256 > 16, this is a breaking change to the internal structure that could affect future extensibility without clear justification in the commit message.
| u8 level; | |
| u16 level; |
| /* | ||
| * Check that quiet masks are subsets of the respective handled masks. | ||
| * Because of the checks above this is sufficient to also ensure that | ||
| * the quiet masks are valid access masks. | ||
| */ | ||
| if ((ruleset_attr.quiet_access_fs | ruleset_attr.handled_access_fs) != | ||
| ruleset_attr.handled_access_fs) | ||
| return -EINVAL; | ||
| if ((ruleset_attr.quiet_access_net | ruleset_attr.handled_access_net) != | ||
| ruleset_attr.handled_access_net) | ||
| return -EINVAL; | ||
| if ((ruleset_attr.quiet_scoped | ruleset_attr.scoped) != | ||
| ruleset_attr.scoped) | ||
| return -EINVAL; |
Copilot
AI
Dec 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The validation logic checks if quiet masks are subsets of handled masks, but uses bitwise OR which is correct. However, there's an inconsistency: for quiet_access_fs and quiet_access_net, the validation only checks they're subsets of the corresponding handled masks. But what if quiet_scoped bits are set without the corresponding scoped bits being set? The same validation pattern should apply to all three pairs. The implementation looks correct but could be clearer with a comment explaining why this is sufficient.
d183aac to
f38b112
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f38b112 to
ec9dd88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @quiet_optional_accesses: Stores which optional accesses are | ||
| * covered by quiet rules within the layer referred to in deny_masks, | ||
| * one access per bit. Does not take into account whether the quiet | ||
| * access bits are actually set in the layer's corresponding | ||
| * landlock_hierarchy. |
Copilot
AI
Dec 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "Does not take into account whether the quiet access bits are actually set in the layer's corresponding landlock_hierarchy." This is confusing because it's not immediately clear why this design choice was made or what the implications are. Consider clarifying when this field should be used and why it doesn't check the hierarchy's quiet_masks.
| * @quiet_optional_accesses: Stores which optional accesses are | |
| * covered by quiet rules within the layer referred to in deny_masks, | |
| * one access per bit. Does not take into account whether the quiet | |
| * access bits are actually set in the layer's corresponding | |
| * landlock_hierarchy. | |
| * @quiet_optional_accesses: Bitmask of optional accesses that are | |
| * covered by quiet rules in the layer referenced by @deny_masks, | |
| * with one access per bit. | |
| * | |
| * This is per-file, cached metadata computed when the file is opened. | |
| * It indicates which optional accesses, if denied by that layer, are | |
| * potentially subject to "quiet" handling for auditing purposes. | |
| * | |
| * This field does not re-check or mirror the layer's | |
| * &struct landlock_hierarchy quiet_masks and therefore must not be | |
| * treated as an authoritative view of the hierarchy configuration. | |
| * Callers that need the exact current quiet state of a hierarchy must | |
| * still consult the hierarchy itself; this field is only a fast hint | |
| * tied to the file's open-time context. |
| /* | ||
| * Leave LANDLOCK_REQUEST_PTRACE and | ||
| * LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are | ||
| * never quiet. | ||
| */ |
Copilot
AI
Dec 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Leave LANDLOCK_REQUEST_PTRACE and LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are never quiet" suggests these request types exist but aren't handled. However, if they are truly never quiet, the code should explicitly set quiet_applicable_to_access to false for these cases rather than leaving it uninitialized (it's initialized to false at line 572, so this is actually safe). Consider adding explicit cases for these types with a comment explaining why they're not quietable, for better code clarity and to avoid relying on the initial false value.
| /* | |
| * Leave LANDLOCK_REQUEST_PTRACE and | |
| * LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are | |
| * never quiet. | |
| */ | |
| case LANDLOCK_REQUEST_PTRACE: | |
| case LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY: | |
| /* | |
| * These request types are never quietable. Make this | |
| * explicit instead of relying on the initial false | |
| * value of quiet_applicable_to_access. | |
| */ | |
| quiet_applicable_to_access = false; | |
| break; |
ec9dd88 to
5fc6d4c
Compare
161db18 to
ef4536f
Compare
5fc6d4c to
65fcf66
Compare
To avoid unnecessarily increasing the size of struct landlock_layer, we make the layer level a u8 and use the space to store the flags struct. Cc: Justin Suess <utilityemal77@gmail.com> Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v7: - Take rule_flags separately from landlock_request in is_access_to_paths_allowed to avoid writing to the landlock_request variable if CONFIG_AUDIT is disabled (to enable compiler elision). - Due to the above change, we don't need rule_flags in landlock_request in this commit anymore (will be added later). Changes in v6: - Rebased to include the revised disconnected directory handling changes (without the "reverting" behaviour) Changes in v5: - Move rule_flags into landlock_request. This lets us get rid of the extra parameters to is_access_to_paths_allowed (and later on, landlock_log_denial), and thus less code changes. Changes in v3: - Comment changes, move local variables, simplify if branch Changes in v2: - Comment changes - Rebased to include disconnected directory handling changes on mic/next and add backing up of collected_rule_flags.
Adds the UAPI for the quiet flags feature (but not the implementation yet). According to pahole, even after adding the struct access_masks quiet_masks in struct landlock_hierarchy, the u32 log_* bitfield still only has a size of 2 bytes, so there's minimal wasted space. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v6: - Fix typo in doc Changes in v5: - Doc fixes. - Fix build failure without CONFIG_AUDIT / CONFIG_INET (reported by Justin Suess) Changes in v4: - Minor update to this commit message. - Fix minor formatting Changes in v3: - Updated docs from Mickaël's suggestions. Changes in v2: - Per suggestion, added support for quieting only certain access bits, controlled by extra quiet_access_* fields in the ruleset_attr. - Added docs for the extra fields and made updates to doc changes in v1. In particular, call out that the effect of LANDLOCK_ADD_RULE_QUIET is independent from the access bits passed in rule_attr - landlock_add_rule will return -EINVAL when LANDLOCK_ADD_RULE_QUIET is used but the ruleset does not have any quiet access bits set for the given rule type. - ABI version bump to v8 - Syntactic and comment changes per suggestion.
The quietness behaviour is as documented in the previous patch. For optional accesses, since the existing deny_masks can only store 2x4bit of layer index, with no way to represent "no layer", we need to either expand it or have another field to correctly handle quieting of those. This commit uses the latter approach - we add another field to store which optional access (of the 2) are covered by quiet rules in their respective layers as stored in deny_masks. We can avoid making struct landlock_file_security larger by converting the existing fown_layer to a 4bit field. This commit does that, and adds test to ensure that it is large enough for LANDLOCK_MAX_NUM_LAYERS-1. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v7: - Following change in commit 1, now we need to copy rule_flags into landlock_request before calling landlock_log_denial for relevant fs denials - Remove left over param comment Changes in v5: - Update code style and comment in get_layer_from_deny_masks() and landlock_log_denial() - Now that rule_flags is moved into landlock_request, this version removes the extra parameter for landlock_log_denial and gets rid of no_rule_flags, simplifying some code. - Fix build failure without CONFIG_AUDIT (reported by Justin Suess) Changes in v3: - Renamed patch title from "Check for quiet flag in landlock_log_denial" to this given the growth. - Moved quiet bit check after domain_exec check - Rename, style and comment fixes suggested by Mickaël. - Squashed patch 6/6 from v2 "Implement quiet for optional accesses" into this one. Changes to that below: - Refactor the quiet flag setting in get_layer_from_deny_masks() to be more clear. - Add KUnit tests - Fix comments, add WARN_ON_ONCE, use __const_hweight64() as suggested by review - Move build_check_file_security to fs.c - Use a typedef for quiet_optional_accesses, add static_assert, and improve docs on landlock_get_quiet_optional_accesses. Changes in v2: - Supports the new quiet access masks. - Support quieting scope requests (but not ptrace and attempted mounting for now)
Adds ability to set which access bits to quiet via LL_*_QUIET_ACCESS (FS,
NET or SCOPED), and attach quiet flags to individual objects via
LL_*_QUIET for FS and NET.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Changes in v6:
- Make populate_ruleset_{fs,net} take a flags argument instead of a bool
quiet (suggested by Justin Suess)
- Fix if braces style
Changes in v3:
- Minor change to the above commit message.
Changes in v2:
- Added new environment variables to control which quiet access bits to
set on the rule, and populate quiet_access_* from it.
- Added support for quieting net rules and scoped access. Renamed patch
title.
- Increment ABI version
The next commit will reuse this number. Make it a shared constant to future-proof changes. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v3: - New patch
Test various interactions of the quiet flag with filesystem rules: - Non-optional access (tested with open and rename). - Optional access (tested with truncate and ioctl). - Behaviour around mounts matches with normal Landlock rules. - Behaviour around disconnected directories matches with normal Landlock rules (test expected behaviour of 9a868cd ("landlock: Fix handling of disconnected directories") applied to the collected quiet flag). - Multiple layers works as expected. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v6: - Change quiet bool argument of add_path_beneath into a __u32 flags (suggested by Justin Suess) - Rename quiet_behind_mountpoint_ignored_disconnected to quiet_behind_mountpoint_disconnected and fix test due to disconnected directory handling changes Changes in v5: - Add quiet_two_layers_different_handled_{1,2,3} variants. Changes in v3: - New patch
Tests that: - Quiet flag works on network rules - Quiet flag applied to unrelated ports has no effect - Denied access not in quiet_access_net is still logged This is not as thorough as the fs tests, but given the shared logic it should be sufficient. There is also no "optional" access for network rules. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v3: - New patch
Enhance scoped_audit.connect_to_child and audit_flags.signal to test interaction with various quiet flag settings. Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v4: - New patch
Signed-off-by: Tingmao Wang <m@maowtm.org> --- Changes in v4: - New patch
landlock-lsm#44
WIP
TODO:
put
struct rule_flags_masksin the audit request