| 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: <Z4WOISi2ZP5kbqcN@google.com>
Date: Mon, 13 Jan 2025 23:05:21 +0100
From: "Günther Noack" <gnoack@...gle.com>
To: "Mickaël Salaün" <mic@...ikod.net>
Cc: Boqun Feng <boqun.feng@...il.com>, Ingo Molnar <mingo@...hat.com>,
Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Matthieu Buffet <matthieu@...fet.re>,
Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, Peter Zijlstra <peterz@...radead.org>,
Shervin Oloumi <enlightened@...omium.org>, Waiman Long <longman@...hat.com>,
Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org
Subject: Re: [PATCH v1 1/4] landlock: Use scoped guards for ruleset
On Mon, Jan 13, 2025 at 05:11:09PM +0100, Mickaël Salaün wrote:
> Simplify error handling by replacing goto statements with automatic
> calls to landlock_put_ruleset() when going out of scope.
>
> This change will be easy to backport to v6.6 if needed, only the
> kernel.h include line conflicts. As for any other similar changes, we
> should be careful when backporting without goto statements.
>
> Add missing include file.
>
> Cc: Günther Noack <gnoack@...gle.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20250113161112.452505-2-mic@digikod.net
> ---
> security/landlock/ruleset.c | 22 ++++++++++------------
> security/landlock/ruleset.h | 5 +++++
> security/landlock/syscalls.c | 25 ++++++++-----------------
> 3 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index a93bdbf52fff..f27b7bdb19b9 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -8,11 +8,13 @@
>
> #include <linux/bits.h>
> #include <linux/bug.h>
> +#include <linux/cleanup.h>
> #include <linux/compiler_types.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/lockdep.h>
> +#include <linux/mutex.h>
> #include <linux/overflow.h>
> #include <linux/rbtree.h>
> #include <linux/refcount.h>
> @@ -537,7 +539,7 @@ struct landlock_ruleset *
> landlock_merge_ruleset(struct landlock_ruleset *const parent,
> struct landlock_ruleset *const ruleset)
> {
> - struct landlock_ruleset *new_dom;
> + struct landlock_ruleset *new_dom __free(landlock_put_ruleset) = NULL;
> u32 num_layers;
> int err;
>
> @@ -557,29 +559,25 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
> new_dom = create_ruleset(num_layers);
> if (IS_ERR(new_dom))
> return new_dom;
> +
> new_dom->hierarchy =
> kzalloc(sizeof(*new_dom->hierarchy), GFP_KERNEL_ACCOUNT);
> - if (!new_dom->hierarchy) {
> - err = -ENOMEM;
> - goto out_put_dom;
> - }
> + if (!new_dom->hierarchy)
> + return ERR_PTR(-ENOMEM);
> +
> refcount_set(&new_dom->hierarchy->usage, 1);
>
> /* ...as a child of @parent... */
> err = inherit_ruleset(parent, new_dom);
> if (err)
> - goto out_put_dom;
> + return ERR_PTR(err);
>
> /* ...and including @ruleset. */
> err = merge_ruleset(new_dom, ruleset);
> if (err)
> - goto out_put_dom;
> -
> - return new_dom;
> + return ERR_PTR(err);
>
> -out_put_dom:
> - landlock_put_ruleset(new_dom);
> - return ERR_PTR(err);
> + return no_free_ptr(new_dom);
> }
>
> /*
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 631e24d4ffe9..70e5b53d1c71 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -11,6 +11,8 @@
>
> #include <linux/bitops.h>
> #include <linux/build_bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/mutex.h>
> #include <linux/rbtree.h>
> @@ -252,6 +254,9 @@ landlock_create_ruleset(const access_mask_t access_mask_fs,
> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>
> +DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *,
> + if (!IS_ERR_OR_NULL(_T)) landlock_put_ruleset(_T))
> +
> int landlock_insert_rule(struct landlock_ruleset *const ruleset,
> const struct landlock_id id,
> const access_mask_t access);
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 4ed8e70c25ed..5a7f1f77292e 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -10,6 +10,7 @@
> #include <linux/anon_inodes.h>
> #include <linux/build_bug.h>
> #include <linux/capability.h>
> +#include <linux/cleanup.h>
> #include <linux/compiler_types.h>
> #include <linux/dcache.h>
> #include <linux/err.h>
> @@ -456,10 +457,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
> SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
> flags)
> {
> - struct landlock_ruleset *new_dom, *ruleset;
> + struct landlock_ruleset *new_dom,
> + *ruleset __free(landlock_put_ruleset) = NULL;
> struct cred *new_cred;
> struct landlock_cred_security *new_llcred;
> - int err;
>
> if (!is_initialized())
> return -EOPNOTSUPP;
> @@ -483,10 +484,9 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
>
> /* Prepares new credentials. */
> new_cred = prepare_creds();
> - if (!new_cred) {
> - err = -ENOMEM;
> - goto out_put_ruleset;
> - }
> + if (!new_cred)
> + return -ENOMEM;
> +
> new_llcred = landlock_cred(new_cred);
>
> /*
> @@ -495,21 +495,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
> */
> new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset);
> if (IS_ERR(new_dom)) {
> - err = PTR_ERR(new_dom);
> - goto out_put_creds;
> + abort_creds(new_cred);
> + return PTR_ERR(new_dom);
> }
>
> /* Replaces the old (prepared) domain. */
> landlock_put_ruleset(new_llcred->domain);
> new_llcred->domain = new_dom;
> -
> - landlock_put_ruleset(ruleset);
> return commit_creds(new_cred);
> -
> -out_put_creds:
> - abort_creds(new_cred);
> -
> -out_put_ruleset:
> - landlock_put_ruleset(ruleset);
> - return err;
> }
> --
> 2.47.1
>
Very nice :)
Reviewed-by: Günther Noack <gnoack@...gle.com>
Powered by blists - more mailing lists