On 08/02/2022 08:55, Konstantin Meskhidze wrote:
2/7/2022 5:17 PM, Mickaël Salaün пишет:
On 07/02/2022 14:09, Konstantin Meskhidze wrote:
2/1/2022 3:13 PM, Mickaël Salaün пишет:
On 24/01/2022 09:02, Konstantin Meskhidze wrote:
Support of socket_bind() and socket_connect() hooks.
Current prototype can restrict binding and connecting of TCP
types of sockets. Its just basic idea how Landlock could support
network confinement.
Changes:
1. Access masks array refactored into 1D one and changed
to 32 bits. Filesystem masks occupy 16 lower bits and network
masks reside in 16 upper bits.
2. Refactor API functions in ruleset.c:
1. Add void *object argument.
2. Add u16 rule_type argument.
3. Use two rb_trees in ruleset structure:
1. root_inode - for filesystem objects
2. root_net_port - for network port objects
It's good to add a changelog, but they must not be in commit
messages that get copied by git am. Please use "---" to separate
this additionnal info (but not the Signed-off-by). Please also
include a version in the email subjects (this one should have been
"[RFC PATCH v3 1/2] landlock: …"), e.g. using git format-patch
--reroll-count=X .
Please follow these rules:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
You can take some inspiration from this patch series:
https://lore.kernel.org/lkml/20210422154123.13086-1-mic@xxxxxxxxxxx/
Ok. I will add patch vervison in next patch. So it will be "[RFC PATCH
v4 ../..] landlock: ..."
But the previous patches remain with no version, correct?
Right, you can't change the subject of already sent emails. ;)
Ok. But I can add previous patches like:
v1:
https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@xxxxxxxxxx
v2:
https://lore.kernel.org/netdev/20211228115212.703084-1-konstantin.meskhidze@xxxxxxxxxx/
v3: ....
right ?
Absolutely! This is a good practice (and would be better in reverse order).
[...]
@@ -67,10 +76,11 @@ static void build_check_rule(void)
}
static struct landlock_rule *create_rule(
- struct landlock_object *const object,
+ void *const object,
Instead of shoehorning two different types into one (and then
loosing the typing), you should rename object to object_ptr and add
a new object_data argument. Only one of these should be set
according to the rule_type. However, if there is no special action
performed on one of these type (e.g. landlock_get_object), only one
uintptr_t argument should be enough.
Do you mean using 2 object arguments in create_rule():
1. create_rule( object_ptr = landlock_object , object_data = 0,
..., fs_rule_type);
2. create_rule( object_ptr = NULL , object_data = port, .... ,
net_rule_type);
Yes, and you can add a WARN_ON_ONCE() in these function to check that
only one argument is set (but object_data could be 0 in each case).
The landlock_get_object() function should only require an object_data
though.
Sorry. As you said in previous comment in landlock_get_object, only
one uintptr_t argument should be enough. But, I did not get: "The
landlock_get_object() function should only require an object_data
though".
uintptr_t is the only argument in landlock_get_object?
I was thinking about landlock_find_rule(), not landlock_get_object():
const struct landlock_rule *landlock_find_rule(
const struct landlock_ruleset *const ruleset,
const uintptr_t object_data)
[...]
@@ -317,47 +331,91 @@ SYSCALL_DEFINE4(landlock_add_rule,
if (flags)
return -EINVAL;
- if (rule_type != LANDLOCK_RULE_PATH_BENEATH)
+ if ((rule_type != LANDLOCK_RULE_PATH_BENEATH) &&
+ (rule_type != LANDLOCK_RULE_NET_SERVICE))
Please replace with a switch/case.
Ok. I got it.
return -EINVAL;
- /* Copies raw user space buffer, only one type for now. */
- res = copy_from_user(&path_beneath_attr, rule_attr,
- sizeof(path_beneath_attr));
- if (res)
- return -EFAULT;
-
- /* Gets and checks the ruleset. */
- ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
- if (IS_ERR(ruleset))
- return PTR_ERR(ruleset);
-
- /*
- * Informs about useless rule: empty allowed_access (i.e. deny
rules)
- * are ignored in path walks.
- */
- if (!path_beneath_attr.allowed_access) {
- err = -ENOMSG;
- goto out_put_ruleset;
- }
- /*
- * Checks that allowed_access matches the @ruleset constraints
- * (ruleset->fs_access_masks[0] is automatically upgraded to
64-bits).
- */
- if ((path_beneath_attr.allowed_access |
ruleset->fs_access_masks[0]) !=
- ruleset->fs_access_masks[0]) {
- err = -EINVAL;
- goto out_put_ruleset;
+ switch (rule_type) {
+ case LANDLOCK_RULE_PATH_BENEATH:
+ /* Copies raw user space buffer, for fs rule type. */
+ res = copy_from_user(&path_beneath_attr, rule_attr,
+ sizeof(path_beneath_attr));
+ if (res)
+ return -EFAULT;
+ break;
+
+ case LANDLOCK_RULE_NET_SERVICE:
+ /* Copies raw user space buffer, for net rule type. */
+ res = copy_from_user(&net_service_attr, rule_attr,
+ sizeof(net_service_attr));
+ if (res)
+ return -EFAULT;
+ break;
}
- /* Gets and checks the new rule. */
- err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
- if (err)
- goto out_put_ruleset;
+ if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
+ /* Gets and checks the ruleset. */
+ ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
+ if (IS_ERR(ruleset))
+ return PTR_ERR(ruleset);
+
+ /*
+ * Informs about useless rule: empty allowed_access (i.e.
deny rules)
+ * are ignored in path walks.
+ */
+ if (!path_beneath_attr.allowed_access) {
+ err = -ENOMSG;
+ goto out_put_ruleset;
+ }
+ /*
+ * 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]) {
+ err = -EINVAL;
+ goto out_put_ruleset;
+ }
+
+ /* Gets and checks the new rule. */
+ err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
+ if (err)
+ goto out_put_ruleset;
+
+ /* Imports the new rule. */
+ err = landlock_append_fs_rule(ruleset, &path,
+ path_beneath_attr.allowed_access);
+ path_put(&path);
+ }
- /* Imports the new rule. */
- err = landlock_append_fs_rule(ruleset, &path,
- path_beneath_attr.allowed_access);
- path_put(&path);
+ if (rule_type == LANDLOCK_RULE_NET_SERVICE) {
+ /* Gets and checks the ruleset. */
+ ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
You need to factor out more code.
Sorry. I did not get you here. Please could you explain more
detailed?
Instead of duplicating similar function calls (e.g.
get_ruleset_from_fd) or operations, try to use one switch statement
where you put the checks that are different (you can move the
copy_from_user(&path_beneath_attr...) call). It may be a good idea to
split this function into 3: one handling each rule_attr, which enables
to not mix different attr types in the same function. A standalone
patch should be refactoring the code to add and use a new function
add_rule_path_beneath(ruleset, rule_attr) (only need the "landlock_"
prefix for exported functions).
Sorry again. Still don't get the point. What function do you suggetst
to split in 3? Can you please give detailed template of these
functions and the logic?
You can split SYSCALL_DEFINE4(landlock_add_rule) in 3:
- a lighten version of SYSCALL_DEFINE4(landlock_add_rule) containing
switch cases for rule_type (almost what you did but with the
get_ruleset_from_fd moved before);
- a new add_rule_path_beneath(ruleset, rule_attr) which will be called
by the switch case;
- a new add_rule_net_service(ruleset, rule_attr) which will be called by
the switch case.
