| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a28c8bec-3671-2613-9107-2b911305c274@digikod.net>
Date: Tue, 15 Mar 2022 18:48:24 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: willemdebruijn.kernel@...il.com,
linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org, yusongping@...wei.com,
artem.kuzin@...wei.com, anton.sirazetdinov@...wei.com
Subject: Re: [RFC PATCH v4 02/15] landlock: filesystem access mask helpers
This patch should be squashed with the previous one. They both refactor
FS access masks in a complementary way.
On 09/03/2022 14:44, Konstantin Meskhidze wrote:
> This patch adds filesystem helper functions
> to set and get filesystem mask. Also the modification
> adds a helper structure landlock_access_mask to
> support managing multiple access mask.
>
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> ---
>
> Changes since v3:
> * Split commit.
> * Add get_mask, set_mask helpers for filesystem.
> * Add new struct landlock_access_mask.
>
> ---
> security/landlock/fs.c | 4 ++--
> security/landlock/ruleset.c | 20 +++++++++++++++++---
> security/landlock/ruleset.h | 19 ++++++++++++++++++-
> security/landlock/syscalls.c | 9 ++++++---
> 4 files changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index d727bdab7840..97f5c455f5a7 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -163,7 +163,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> return -EINVAL;
>
> /* Transforms relative access rights to absolute ones. */
> - access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->access_masks[0];
> + access_rights |= LANDLOCK_MASK_ACCESS_FS & ~landlock_get_fs_access_mask(ruleset, 0);
> object = get_inode_object(d_backing_inode(path->dentry));
> if (IS_ERR(object))
> return PTR_ERR(object);
> @@ -252,7 +252,7 @@ static int check_access_path(const struct landlock_ruleset *const domain,
> /* Saves all layers handling a subset of requested accesses. */
> layer_mask = 0;
> for (i = 0; i < domain->num_layers; i++) {
> - if (domain->access_masks[i] & access_request)
> + if (landlock_get_fs_access_mask(domain, i) & access_request)
> layer_mask |= BIT_ULL(i);
> }
> /* An access request not handled by the domain is allowed. */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 78341a0538de..a6212b752549 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -44,16 +44,30 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> return new_ruleset;
> }
>
> -struct landlock_ruleset *landlock_create_ruleset(const u32 access_mask)
> +/* A helper function to set a filesystem mask */
> +void landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
struct landlock_ruleset *const ruleset
Please use const as much as possible even in function arguments: e.g.
access_masks_set, mask_level…
> + const struct landlock_access_mask *access_mask_set,
nit: no need for "_set" suffix.
Why do you need a struct landlock_access_mask and not just u16 (which
will probably become a subset of access_mask_t, see [1])?
landlock_create_ruleset() could just take two masks as argument instead.
[1] https://lore.kernel.org/all/20220221212522.320243-2-mic@digikod.net/
> + u16 mask_level)
> +{
> + ruleset->access_masks[mask_level] = access_mask_set->fs;
> +}
> +
> +/* A helper function to get a filesystem mask */
> +u32 landlock_get_fs_access_mask(const struct landlock_ruleset *ruleset, u16 mask_level)
> +{
> + return ruleset->access_masks[mask_level];
> +}
You can move these two helpers to ruleset.h and make them static inline.
> +
> +struct landlock_ruleset *landlock_create_ruleset(const struct landlock_access_mask *access_mask_set)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!access_mask)
> + if (!access_mask_set->fs)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (!IS_ERR(new_ruleset))
> - new_ruleset->access_masks[0] = access_mask;
> + landlock_set_fs_access_mask(new_ruleset, access_mask_set, 0);
> return new_ruleset;
> }
>
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 32d90ce72428..bc87e5f787f7 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -16,6 +16,16 @@
>
> #include "object.h"
>
> +/**
> + * struct landlock_access_mask - A helper structure to handle different mask types
> + */
> +struct landlock_access_mask {
> + /**
> + * @fs: Filesystem access mask.
> + */
> + u16 fs;
> +};
Removing this struct would simplify the code.
> +
> /**
> * struct landlock_layer - Access rights for a given layer
> */
> @@ -140,7 +150,8 @@ struct landlock_ruleset {
> };
> };
>
> -struct landlock_ruleset *landlock_create_ruleset(const u32 access_mask);
> +struct landlock_ruleset *landlock_create_ruleset(const struct landlock_access_mask
> + *access_mask_set);
>
> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -162,4 +173,10 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
> refcount_inc(&ruleset->usage);
> }
>
> +void landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
> + const struct landlock_access_mask *access_mask_set,
> + u16 mask_level);
> +
> +u32 landlock_get_fs_access_mask(const struct landlock_ruleset *ruleset, u16 mask_level);
> +
> #endif /* _SECURITY_LANDLOCK_RULESET_H */
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index f1d86311df7e..5931b666321d 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -159,6 +159,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> {
> struct landlock_ruleset_attr ruleset_attr;
> struct landlock_ruleset *ruleset;
> + struct landlock_access_mask access_mask_set = {.fs = 0};
> int err, ruleset_fd;
>
> /* Build-time checks. */
> @@ -185,9 +186,10 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
> LANDLOCK_MASK_ACCESS_FS)
> return -EINVAL;
> + access_mask_set.fs = ruleset_attr.handled_access_fs;
>
> /* Checks arguments and transforms to kernel struct. */
> - ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs);
> + ruleset = landlock_create_ruleset(&access_mask_set);
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
>
> @@ -343,8 +345,9 @@ SYSCALL_DEFINE4(landlock_add_rule,
> * 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]) {
> +
> + if ((path_beneath_attr.allowed_access | landlock_get_fs_access_mask(ruleset, 0)) !=
> + landlock_get_fs_access_mask(ruleset, 0)) {
> err = -EINVAL;
> goto out_put_ruleset;
> }
> --
> 2.25.1
>
Powered by blists - more mailing lists