Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On 24/03/2021 21:31, James Morris wrote: > On Fri, 19 Mar 2021, Mickaël Salaün wrote: > >> Cc: Kees Cook Signed-off-by: Mickaël Salaün Acked-by: Serge Hallyn Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net >>> >>> (Aside: you appear to be self-adding your Link: tags -- AIUI, this is >>> normally done by whoever pulls your series. I've only seen Link: tags >>> added when needing to refer to something else not included in the >>> series.) >> >> It is an insurance to not lose history. :) > > How will history be lost? The code is in the repo and discussions can > easily be found by searching for subjects or message IDs. The (full and ordered) history may be hard to find without any Message-ID in commit messages. The Lore links keep that information (in the commit message) and redirect to the related archived email thread, which is very handy. For instance, Linus can rely on those links to judge the quality of a patch: https://lore.kernel.org/lkml/CAHk-=wh7xY3UF7zEc0BNVNjOox59jYBW-Gfi7=emm+bxpwc...@mail.gmail.com/ > > Is anyone else doing this self linking? > I don't know, but it doesn't hurt. This way, if you're using git am without b4 am -l (or forgot to add links manually), the history is still pointed out by these self-reference links. I find it convenient and it is a safeguard to not forget them, no matter who takes the patches.
Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On Fri, 19 Mar 2021, Mickaël Salaün wrote: > > >> Cc: Kees Cook > >> Signed-off-by: Mickaël Salaün > >> Acked-by: Serge Hallyn > >> Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net > > > > (Aside: you appear to be self-adding your Link: tags -- AIUI, this is > > normally done by whoever pulls your series. I've only seen Link: tags > > added when needing to refer to something else not included in the > > series.) > > It is an insurance to not lose history. :) How will history be lost? The code is in the repo and discussions can easily be found by searching for subjects or message IDs. Is anyone else doing this self linking? -- James Morris
Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On Tue, Mar 16, 2021 at 9:43 PM Mickaël Salaün wrote: > A Landlock ruleset is mainly a red-black tree with Landlock rules as > nodes. This enables quick update and lookup to match a requested > access, e.g. to a file. A ruleset is usable through a dedicated file > descriptor (cf. following commit implementing syscalls) which enables a > process to create and populate a ruleset with new rules. > > A domain is a ruleset tied to a set of processes. This group of rules > defines the security policy enforced on these processes and their future > children. A domain can transition to a new domain which is the > intersection of all its constraints and those of a ruleset provided by > the current process. This modification only impact the current process. > This means that a process can only gain more constraints (i.e. lose > accesses) over time. > > Cc: James Morris > Cc: Jann Horn > Cc: Kees Cook > Signed-off-by: Mickaël Salaün > Acked-by: Serge Hallyn > Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net Reviewed-by: Jann Horn
Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On Fri, Mar 19, 2021 at 08:03:22PM +0100, Mickaël Salaün wrote:
> On 19/03/2021 19:40, Kees Cook wrote:
> > On Tue, Mar 16, 2021 at 09:42:42PM +0100, Mickaël Salaün wrote:
> >> [...]
> >> +static void put_rule(struct landlock_rule *const rule)
> >> +{
> >> + might_sleep();
> >> + if (!rule)
> >> + return;
> >> + landlock_put_object(rule->object);
> >> + kfree(rule);
> >> +}
> >
> > I'd expect this to be named "release" rather than "put" since it doesn't
> > do any lifetime reference counting.
>
> It does decrement rule->object->usage .
Well, landlock_put_object() decrements rule->object's lifetime. It seems
"rule" doesn't have a lifetime. (There is no refcounter on rule.) I just
find it strange to see "put" without a matching "get". Not a big deal.
--
Kees Cook
Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On 19/03/2021 19:40, Kees Cook wrote:
> On Tue, Mar 16, 2021 at 09:42:42PM +0100, Mickaël Salaün wrote:
>> From: Mickaël Salaün
>>
>> A Landlock ruleset is mainly a red-black tree with Landlock rules as
>> nodes. This enables quick update and lookup to match a requested
>> access, e.g. to a file. A ruleset is usable through a dedicated file
>> descriptor (cf. following commit implementing syscalls) which enables a
>> process to create and populate a ruleset with new rules.
>>
>> A domain is a ruleset tied to a set of processes. This group of rules
>> defines the security policy enforced on these processes and their future
>> children. A domain can transition to a new domain which is the
>> intersection of all its constraints and those of a ruleset provided by
>> the current process. This modification only impact the current process.
>> This means that a process can only gain more constraints (i.e. lose
>> accesses) over time.
>>
>> Cc: James Morris
>> Cc: Jann Horn
>> Cc: Kees Cook
>> Signed-off-by: Mickaël Salaün
>> Acked-by: Serge Hallyn
>> Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net
>
> (Aside: you appear to be self-adding your Link: tags -- AIUI, this is
> normally done by whoever pulls your series. I've only seen Link: tags
> added when needing to refer to something else not included in the
> series.)
It is an insurance to not lose history. :)
>
>> [...]
>> +static void put_rule(struct landlock_rule *const rule)
>> +{
>> +might_sleep();
>> +if (!rule)
>> +return;
>> +landlock_put_object(rule->object);
>> +kfree(rule);
>> +}
>
> I'd expect this to be named "release" rather than "put" since it doesn't
> do any lifetime reference counting.
It does decrement rule->object->usage .
>
>> +static void build_check_ruleset(void)
>> +{
>> +const struct landlock_ruleset ruleset = {
>> +.num_rules = ~0,
>> +.num_layers = ~0,
>> +};
>> +
>> +BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
>> +BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>> +}
>
> This is checking that the largest possible stored value is correctly
> within the LANDLOCK_MAX_* macro value?
Yes, there is builtin checks for all Landlock limits.
>
>> [...]
>
> The locking all looks right, and given your test coverage and syzkaller
> work, it's hard for me to think of ways to prove it out any better. :)
Thanks!
>
> Reviewed-by: Kees Cook
>
>
Re: [PATCH v30 02/12] landlock: Add ruleset and domain management
On Tue, Mar 16, 2021 at 09:42:42PM +0100, Mickaël Salaün wrote:
> From: Mickaël Salaün
>
> A Landlock ruleset is mainly a red-black tree with Landlock rules as
> nodes. This enables quick update and lookup to match a requested
> access, e.g. to a file. A ruleset is usable through a dedicated file
> descriptor (cf. following commit implementing syscalls) which enables a
> process to create and populate a ruleset with new rules.
>
> A domain is a ruleset tied to a set of processes. This group of rules
> defines the security policy enforced on these processes and their future
> children. A domain can transition to a new domain which is the
> intersection of all its constraints and those of a ruleset provided by
> the current process. This modification only impact the current process.
> This means that a process can only gain more constraints (i.e. lose
> accesses) over time.
>
> Cc: James Morris
> Cc: Jann Horn
> Cc: Kees Cook
> Signed-off-by: Mickaël Salaün
> Acked-by: Serge Hallyn
> Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net
(Aside: you appear to be self-adding your Link: tags -- AIUI, this is
normally done by whoever pulls your series. I've only seen Link: tags
added when needing to refer to something else not included in the
series.)
> [...]
> +static void put_rule(struct landlock_rule *const rule)
> +{
> + might_sleep();
> + if (!rule)
> + return;
> + landlock_put_object(rule->object);
> + kfree(rule);
> +}
I'd expect this to be named "release" rather than "put" since it doesn't
do any lifetime reference counting.
> +static void build_check_ruleset(void)
> +{
> + const struct landlock_ruleset ruleset = {
> + .num_rules = ~0,
> + .num_layers = ~0,
> + };
> +
> + BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
> + BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
> +}
This is checking that the largest possible stored value is correctly
within the LANDLOCK_MAX_* macro value?
> [...]
The locking all looks right, and given your test coverage and syzkaller
work, it's hard for me to think of ways to prove it out any better. :)
Reviewed-by: Kees Cook
--
Kees Cook
[PATCH v30 02/12] landlock: Add ruleset and domain management
From: Mickaël Salaün
A Landlock ruleset is mainly a red-black tree with Landlock rules as
nodes. This enables quick update and lookup to match a requested
access, e.g. to a file. A ruleset is usable through a dedicated file
descriptor (cf. following commit implementing syscalls) which enables a
process to create and populate a ruleset with new rules.
A domain is a ruleset tied to a set of processes. This group of rules
defines the security policy enforced on these processes and their future
children. A domain can transition to a new domain which is the
intersection of all its constraints and those of a ruleset provided by
the current process. This modification only impact the current process.
This means that a process can only gain more constraints (i.e. lose
accesses) over time.
Cc: James Morris
Cc: Jann Horn
Cc: Kees Cook
Signed-off-by: Mickaël Salaün
Acked-by: Serge Hallyn
Link: https://lore.kernel.org/r/20210316204252.427806-3-...@digikod.net
---
Changes since v28:
* Add Acked-by Serge Hallyn.
* Clean up comment.
Changes since v27:
* Fix domains with layers of non-overlapping access rights.
* Add stricter limit checks (same semantic).
* Change the grow direction of a rule layer stack to make it the same as
the new ruleset fs_access_masks stack (cosmetic change).
* Cosmetic fix for a comment block.
Changes since v26:
* Fix spelling.
Changes since v25:
* Add build-time checks for the num_layers and num_rules variables
according to LANDLOCK_MAX_NUM_LAYERS and LANDLOCK_MAX_NUM_RULES, and
move these limits to a dedicated file.
* Cosmetic variable renames.
Changes since v24:
* Update struct landlock_rule with a layer stack. This reverts "Always
intersect access rights" from v24 and also adds the ability to tie
access rights with their policy layer. As noted by Jann Horn, always
intersecting access rights made some use cases uselessly more
difficult to handle in user space. Thanks to this new stack, we still
have a deterministic policy behavior whatever their level in the stack
of policies, while using a "union" of accesses when building a
ruleset. The implementation use a FAM to keep the access checks quick
and memory efficient (4 bytes per layer per inode). Update
insert_rule() accordingly.
Changes since v23:
* Always intersect access rights. Following the filesystem change
logic, make ruleset updates more consistent by always intersecting
access rights (boolean AND) instead of combining them (boolean OR) for
the same layer. This defensive approach could also help avoid user
space to inadvertently allow multiple access rights for the same
object (e.g. write and execute access on a path hierarchy) instead of
dealing with such inconsistency. This can happen when there is no
deduplication of objects (e.g. paths and underlying inodes) whereas
they get different access rights with landlock_add_rule(2).
* Add extra checks to make sure that:
- there is always an (allocated) object in each used rules;
- when updating a ruleset with a new rule (i.e. not merging two
rulesets), the ruleset doesn't contain multiple layers.
* Hide merge parameter from the public landlock_insert_rule() API. This
helps avoid misuse of this function.
* Replace a remaining hardcoded 1 with SINGLE_DEPTH_NESTING.
Changes since v22:
* Explicitely use RB_ROOT and SINGLE_DEPTH_NESTING (suggested by Jann
Horn).
* Improve comments and fix spelling (suggested by Jann Horn).
Changes since v21:
* Add and clean up comments.
Changes since v18:
* Account rulesets to kmemcg.
* Remove struct holes.
* Cosmetic changes.
Changes since v17:
* Move include/uapi/linux/landlock.h and _LANDLOCK_ACCESS_FS_* to a
following patch.
Changes since v16:
* Allow enforcement of empty ruleset, which enables deny-all policies.
Changes since v15:
* Replace layer_levels and layer_depth with a bitfield of layers, cf.
filesystem commit.
* Rename the LANDLOCK_ACCESS_FS_{UNLINK,RMDIR} with
LANDLOCK_ACCESS_FS_REMOVE_{FILE,DIR} because it makes sense to use
them for the action of renaming a file or a directory, which may lead
to the removal of the source file or directory. Removes the
LANDLOCK_ACCESS_FS_{LINK_TO,RENAME_FROM,RENAME_TO} which are now
replaced with LANDLOCK_ACCESS_FS_REMOVE_{FILE,DIR} and
LANDLOCK_ACCESS_FS_MAKE_* .
* Update the documentation accordingly and highlight how the access
rights are taken into account.
* Change nb_rules from atomic_t to u32 because it is not use anymore by
show_fdinfo().
* Add safeguard for level variables types.
* Check max number of rules.
* Replace struct landlock_access (self and beneath bitfields) with one
bitfield.
* Remove useless variable.
* Add comments.
Changes since v14:
* Simplify the object, rule and ruleset management at the expense of a
less aggressive memory freeing (contributed by Jann Horn, with
additional modifications):
- Make a domain immutable (remove the opportunistic cleaning).
- Remove RCU
