Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Sep 22, 2025 at 12:52:19AM +0100, Tingmao Wang wrote:
> On 9/19/25 17:02, Mickaël Salaün wrote:
> > On Tue, Sep 09, 2025 at 01:06:35AM +0100, Tingmao Wang wrote:
> >> 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.
> >>
> >> Signed-off-by: Tingmao Wang <m@maowtm.org>
> >> ---
> >>  security/landlock/fs.c      | 75 ++++++++++++++++++++++++-------------
> >>  security/landlock/net.c     |  3 +-
> >>  security/landlock/ruleset.c |  9 ++++-
> >>  security/landlock/ruleset.h | 27 ++++++++++++-
> >>  4 files changed, 83 insertions(+), 31 deletions(-)
> >>
> >> [...]
> >> @@ -643,6 +644,12 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
> >>  		unsigned long access_bit;
> >>  		bool is_empty;
> >>  
> >> +		if (rule_flags) {
> >> +			/* Collect rule flags for each layer */
> >> +			if (layer->flags.quiet)
> >> +				rule_flags->quiet_masks |= layer_bit;
> > 
> > This patch makes quiet flags related to on object, but not tied to the
> > rule access rights (as explained in the next patch's doc). To tie it to
> > rule access rights would require to duplicate the access bits for each
> > rule (because multiple rules can be tied to the same object for the same
> > layer/ruleset).
> 
> (imo the use of "rule" here as a terminology is a bit confusing, I would
> have thought that a "rule" is a collection of access rights associated
> with an object, and therefore you can of course only have one rule per
> object in a ruleset.  Otherwise landlock_add_rule should really have been
> called landlock_add_rules...?)

A rule is indeed a set of access rights tied to an object.  However,
when the same object is used with two calls to landlock_add_rule(), the
ruleset's rule is the union of both access rights.

What I wanted to highlight is that the quiet flag is unrelated to the
access rights, which makes sense because the otherwise it would never be
denied and then never logged.

> 
> > 
> > So, the question is, what do we really need to mute?
> > 
> > I think the current approach is enough. We could still add a new flag in
> > the future, or maybe even a new field to each rule type.  However, we
> > should rename the flag to make it clear that the it's related to the
> > rule's object which is muted instead of the whole rule.  Maybe something
> > like LANDLOCK_ADD_RULE_QUIET_OBJECT?
> 
> I don't see what benefit a new field to each rule type would bring, since
> different rule types targets different objects and live in different
> rbtrees, and so they are already separable.

Adding a new field to rule types would allow to only mute denied
requests if they match an access right, instead of muting all
non-allowed access rights.  For instance, if an app tries to do an ioctl
because of legacy/compatibility reasons, we may want to ignore such
request, but we may still want to log malicious attempts to modify
regular files (in the same file hierarchy).

> 
> > 
> > If we want to have a more fine-grained control, a complementary patch
> > could add a bitfield for each access right type to quiet a denied access
> > right iff the object is also quiet (where rules are possible).  That
> > could be a follow up to complete this quiet feature, but this patch
> > series is good on its own.
> 
> Worth noting that if one really wants to suppress logging for only some
> access bits and we do not add support for it (due to the extra overhead
> etc), that is still doable with just this patch by using two layers - the

This hack should definitely not be encouraged, otherwise the limit of 16
layers will shrink to 8 (or less) in practice.

> outer one would contain the intended rules but not have any quiet flags,
> whereas the inner one would contain the same set of rules but with quiet
> flags set, except that for access bits which the sandbox does not want to
> be quiet, it would also "allow" them in the second layer (access would
> still be denied by the first layer, but would get audit logged due to
> quiet flag not applying when the younger layer allows the access).
> 
> But this gets very tricky in the context of mutable domains, and does not
> work at all for the purpose of controlling whether supervisor mode would
> delegate to the supervisor or deny outright, since supervisors are
> "accumulative".  Therefore if this (different "quietness" for different
> access bits) becomes a strong need, we should probably consider some way
> of implementing it, rather than expecting a sandboxer to do this two-layer

> hack.  (But implementing this does have the potential to result in needing
> to have a (number of access bits) x (number of layers) matrix...)

Yes, that will indeed increase the size of rules, which is why I'm not
sure if it worth it.

The alternative I was thinking about is to only increase the size of
struct landlock_ruleset (which would be negligible) to include sets of
quiet access rights.  A request to such access right *on* a quiet object
would never be logged.  I think this approach is flexible enough and
doesn't increase much the complexity.  This would also be useful to not
log access rights that don't have associated rules (e.g. scopes), and
then no identified objects.  To avoid the kind of hack you pointed out,
this feature should probably be part of this patch series though.  What
do you think?

> 
> > [...]
> 
> Will add suggested changes in v2.  Thanks for the review :)
> 
> Kind regards,
> Tingmao
>