[PATCH v3 1/3] landlock: Refactor filesystem access mask management
Mickaël Salaün
mic at digikod.net
Sat Nov 9 11:08:06 UTC 2024
On Thu, Oct 24, 2024 at 04:58:29PM +0200, Günther Noack wrote:
> The approach of only using the union locally to the merging functions is much
> nicer. 👍 Still some documentation/wording remarks, but overall looks good.
>
> On Tue, Oct 22, 2024 at 05:11:42PM +0200, Mickaël Salaün wrote:
> > Replace get_raw_handled_fs_accesses() with a generic
> > landlock_merge_access_masks(), and replace get_fs_domain() with a
> > generic landlock_match_ruleset(). These helpers will also be useful for
> > other types of access.
> >
> > Cc: Günther Noack <gnoack at google.com>
> > Cc: Mikhail Ivanov <ivanov.mikhail1 at huawei-partners.com>
> > Signed-off-by: Mickaël Salaün <mic at digikod.net>
> > Link: https://lore.kernel.org/r/20241022151144.872797-2-mic@digikod.net
> > ---
> >
> > Changes since v2:
> > * Create a new type union access_masks_all instead of changing struct
> > acces_masks.
> > * Replace get_fs_domain() with an explicit call to
> > landlock_match_ruleset().
> >
> > Changes since v1:
> > * Rename landlock_filter_access_masks() to landlock_match_ruleset().
> > * Rename local variables from domain to ruleset to mach helpers'
> > semantic. We'll rename and move these helpers when we'll have a
> > dedicated domain struct type.
> > * Rename the all_fs mask to any_fs.
> > * Add documentation, as suggested by Günther.
> > ---
> > security/landlock/fs.c | 31 ++++------------
> > security/landlock/ruleset.h | 70 +++++++++++++++++++++++++++++++-----
> > security/landlock/syscalls.c | 2 +-
> > 3 files changed, 70 insertions(+), 33 deletions(-)
> >
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 7d79fc8abe21..dd9a7297ea4e 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -388,38 +388,21 @@ static bool is_nouser_or_private(const struct dentry *dentry)
> > unlikely(IS_PRIVATE(d_backing_inode(dentry))));
> > }
> >
> > -static access_mask_t
> > -get_raw_handled_fs_accesses(const struct landlock_ruleset *const domain)
> > -{
> > - access_mask_t access_dom = 0;
> > - size_t layer_level;
> > -
> > - for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
> > - access_dom |=
> > - landlock_get_raw_fs_access_mask(domain, layer_level);
> > - return access_dom;
> > -}
> > -
> > static access_mask_t
> > get_handled_fs_accesses(const struct landlock_ruleset *const domain)
> > {
> > /* Handles all initially denied by default access rights. */
> > - return get_raw_handled_fs_accesses(domain) |
> > + return landlock_merge_access_masks(domain).fs |
> > LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
> > }
> >
> > -static const struct landlock_ruleset *
> > -get_fs_domain(const struct landlock_ruleset *const domain)
> > -{
> > - if (!domain || !get_raw_handled_fs_accesses(domain))
> > - return NULL;
> > -
> > - return domain;
> > -}
> > +static const struct access_masks any_fs = {
> > + .fs = ~0,
> > +};
> >
> > static const struct landlock_ruleset *get_current_fs_domain(void)
> > {
> > - return get_fs_domain(landlock_get_current_domain());
> > + return landlock_match_ruleset(landlock_get_current_domain(), any_fs);
> > }
> >
> > /*
> > @@ -1516,8 +1499,8 @@ static int hook_file_open(struct file *const file)
> > layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > access_mask_t open_access_request, full_access_request, allowed_access,
> > optional_access;
> > - const struct landlock_ruleset *const dom =
> > - get_fs_domain(landlock_cred(file->f_cred)->domain);
> > + const struct landlock_ruleset *const dom = landlock_match_ruleset(
> > + landlock_cred(file->f_cred)->domain, any_fs);
> >
> > if (!dom)
> > return 0;
> > diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> > index 61bdbc550172..e00edcb38c5b 100644
> > --- a/security/landlock/ruleset.h
> > +++ b/security/landlock/ruleset.h
> > @@ -47,6 +47,15 @@ struct access_masks {
> > access_mask_t scope : LANDLOCK_NUM_SCOPE;
> > };
> >
> > +union access_masks_all {
> > + struct access_masks masks;
> > + u32 all;
> > +};
> > +
> > +/* Makes sure all fields are covered. */
> > +static_assert(sizeof(((union access_masks_all *)NULL)->masks) ==
> > + sizeof(((union access_masks_all *)NULL)->all));
>
> Nit: Could maybe be written as
> sizeof(typeof_member(union access_masks_all, masks))
yep
>
> Nit 2: Should this be <= instead of ==?
This would be correct, but I prefer to be stricter to catch any
potential future change.
>
> > +
> > typedef u16 layer_mask_t;
> > /* Makes sure all layers can be checked. */
> > static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
> > @@ -260,6 +269,58 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
> > refcount_inc(&ruleset->usage);
> > }
> >
> > +/**
> > + * landlock_merge_access_masks - Return the merge of a ruleset's access_masks
>
> Documentation uses the same words as in the function name, and it's not
> intuitively clear to me what "merge" means. Would it be fair to describe it
> like this:
>
> landlock_merge_access_masks - Return all access rights handled in the ruleset
>
> ?
Sounds good.
>
> (To describe mathematical set operations, I'd normally say "a union" instead of
> "a merge", but that might be confusing given that we also use the C "union"
> feature in the same function.)
landlock_union_access_masks() looks good to me.
>
> > + *
> > + * @ruleset: Landlock ruleset (used as a domain)
> > + *
> > + * Returns: an access_masks result of the OR of all the ruleset's access masks.
> > + */
> > +static inline struct access_masks
> > +landlock_merge_access_masks(const struct landlock_ruleset *const ruleset)
> > +{
> > + union access_masks_all matches = {};
> > + size_t layer_level;
> > +
> > + for (layer_level = 0; layer_level < ruleset->num_layers;
> > + layer_level++) {
> > + union access_masks_all layer = {
> > + .masks = ruleset->access_masks[layer_level],
> > + };
> > +
> > + matches.all |= layer.all;
> > + }
> > +
> > + return matches.masks;
> > +}
> > +
> > +/**
> > + * landlock_match_ruleset - Return @ruleset if any @masks right matches
>
> Same here; I think when I see a call for a function called
> "landlock_match_ruleset" I might be confused about what is being matched against
> what here. Documentation uses the same wording as well. Documentation
> suggestion:
>
> landlock_match_ruleset - Return @ruleset iff any access right in @masks
> is handled in the @ruleset.
>
> This is why in [1], I suggested that this function could return a boolean
> and be called differently, like:
>
> /* True if any access right in @masks is handled in @ruleset. */
> bool landlock_is_any_access_right_handled(
> const struct landlock_ruleset *const ruleset,
> struct access_masks masks);
>
> Returning a boolean removes the (slightly unintuitive) semantics where a
> function argument is returned under certain conditions, which are not clear from
> the function name, and then the function has the more conventional style of
> returning a boolean that indicates whether some condition holds. The function
> name would spell out more exactly what is matched against what.
I get your point but I prefer an helper that limits code verbosity and
potential misuse (which is subjective, but still). With
landlock_match_ruleset(), I think it's easier to check that this
function is called whenever necessary to "match" access masks. This
pattern is then used in almost every hook implementations.
>
> Callers would have to check the boolean and return the ruleset themselves, but
> this seems like a reasonable thing to do when the code is clearer to read, IMHO.
>
> if (landlock_is_any_access_right_handled(ruleset, masks))
> return ruleset;
> return NULL;
I understand but I prefer to simplify future contributions.
>
> Alternatively, how about wording it like this:
>
> /*
> * landlock_get_applicable_domain - Returns the @dom ruleset if it
> * applies to (handles) the access
> * rights specified in @masks.
> */
> const struct landlock_ruleset *landlock_get_applicable_domain(
> const struct landlock_ruleset *const dom,
> const struct access_masks masks);
OK, I'll go with that.
>
> [1] https://lore.kernel.org/all/20241005.a69458234f74@gnoack.org/
>
>
> > + *
> > + * @ruleset: Landlock ruleset (used as a domain)
> > + * @masks: access masks
> > + *
> > + * Returns: @ruleset if @masks matches, or NULL otherwise.
> > + */
> > +static inline const struct landlock_ruleset *
> > +landlock_match_ruleset(const struct landlock_ruleset *const ruleset,
> > + const struct access_masks masks)
> > +{
> > + const union access_masks_all masks_all = {
> > + .masks = masks,
> > + };
> > + union access_masks_all merge = {};
> > +
> > + if (!ruleset)
> > + return NULL;
> > +
> > + merge.masks = landlock_merge_access_masks(ruleset);
> > + if (merge.all & masks_all.all)
> > + return ruleset;
> > +
> > + return NULL;
> > +}
> > +
> > static inline void
> > landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
> > const access_mask_t fs_access_mask,
> > @@ -295,19 +356,12 @@ landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
> > ruleset->access_masks[layer_level].scope |= mask;
> > }
> >
> > -static inline access_mask_t
> > -landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
> > - const u16 layer_level)
> > -{
> > - return ruleset->access_masks[layer_level].fs;
> > -}
> > -
> > static inline access_mask_t
> > landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
> > const u16 layer_level)
> > {
> > /* Handles all initially denied by default access rights. */
> > - return landlock_get_raw_fs_access_mask(ruleset, layer_level) |
> > + return ruleset->access_masks[layer_level].fs |
> > LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
> > }
> >
> > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > index f5a0e7182ec0..c097d356fa45 100644
> > --- a/security/landlock/syscalls.c
> > +++ b/security/landlock/syscalls.c
> > @@ -329,7 +329,7 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
> > return -ENOMSG;
> >
> > /* Checks that allowed_access matches the @ruleset constraints. */
> > - mask = landlock_get_raw_fs_access_mask(ruleset, 0);
> > + mask = ruleset->access_masks[0].fs;
> > if ((path_beneath_attr.allowed_access | mask) != mask)
> > return -EINVAL;
> >
> > --
> > 2.47.0
> >
More information about the Linux-security-module-archive
mailing list