[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAG48ez2Sw0Cy3RYrgrsEDKyWoxMmMbzX6yY-OEfZqeyGDQhy9w@mail.gmail.com>
Date: Thu, 15 Aug 2024 22:25:15 +0200
From: Jann Horn <jannh@...gle.com>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: outreachy@...ts.linux.dev, mic@...ikod.net, gnoack@...gle.com,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
bjorn3_gh@...tonmail.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal
scoping support
On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@...il.com> wrote:
> This patch adds two new hooks "hook_file_set_fowner" and
> "hook_file_free_security" to set and release a pointer to the
> domain of the file owner. This pointer "fown_domain" in
> "landlock_file_security" will be used in "file_send_sigiotask"
> to check if the process can send a signal.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@...il.com>
> ---
> security/landlock/fs.c | 18 ++++++++++++++++++
> security/landlock/fs.h | 6 ++++++
> security/landlock/task.c | 27 +++++++++++++++++++++++++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 7877a64cc6b8..d05f0e9c5e54 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
> return -EACCES;
> }
>
> +static void hook_file_set_fowner(struct file *file)
> +{
> + write_lock_irq(&file->f_owner.lock);
Before updating landlock_file(file)->fown_domain, this hook must also
drop a reference on the old domain - maybe by just calling
landlock_put_ruleset_deferred(landlock_file(file)->fown_domain) here.
> + landlock_file(file)->fown_domain = landlock_get_current_domain();
> + landlock_get_ruleset(landlock_file(file)->fown_domain);
> + write_unlock_irq(&file->f_owner.lock);
> +}
> +
> +static void hook_file_free_security(struct file *file)
> +{
> + write_lock_irq(&file->f_owner.lock);
> + landlock_put_ruleset(landlock_file(file)->fown_domain);
> + write_unlock_irq(&file->f_owner.lock);
> +}
> +
> static struct security_hook_list landlock_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
>
> @@ -1660,6 +1675,9 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
> LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
> +
> + LSM_HOOK_INIT(file_set_fowner, hook_file_set_fowner),
> + LSM_HOOK_INIT(file_free_security, hook_file_free_security),
> };
>
> __init void landlock_add_fs_hooks(void)
> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index 488e4813680a..6054563295d8 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -52,6 +52,12 @@ struct landlock_file_security {
> * needed to authorize later operations on the open file.
> */
> access_mask_t allowed_access;
> + /**
> + * @fown_domain: A pointer to a &landlock_ruleset of the process own
> + * the file. This ruleset is protected by fowner_struct.lock same as
> + * pid, uid, euid fields in fown_struct.
> + */
> + struct landlock_ruleset *fown_domain;
> };
>
> /**
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 9de96a5005c4..568292dbfe7d 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -18,6 +18,7 @@
>
> #include "common.h"
> #include "cred.h"
> +#include "fs.h"
> #include "ruleset.h"
> #include "setup.h"
> #include "task.h"
> @@ -261,12 +262,38 @@ static int hook_task_kill(struct task_struct *const p,
> return 0;
> }
>
> +static int hook_file_send_sigiotask(struct task_struct *tsk,
> + struct fown_struct *fown, int signum)
> +{
> + struct file *file;
> + bool is_scoped;
> + const struct landlock_ruleset *dom, *target_dom;
> +
> + /* struct fown_struct is never outside the context of a struct file */
> + file = container_of(fown, struct file, f_owner);
> +
> + read_lock_irq(&file->f_owner.lock);
> + dom = landlock_file(file)->fown_domain;
> + read_unlock_irq(&file->f_owner.lock);
At this point, the ->fown_domain pointer could concurrently change,
and (once you apply my suggestion above) the old ->fown_domain could
therefore be freed concurrently. One way to avoid that would be to use
landlock_get_ruleset() to grab a reference before calling
read_unlock_irq(), and drop that reference with
landlock_put_ruleset_deferred() before exiting from this function.
> + if (!dom)
> + return 0;
> +
> + rcu_read_lock();
> + target_dom = landlock_get_task_domain(tsk);
> + is_scoped = domain_is_scoped(dom, target_dom, LANDLOCK_SCOPED_SIGNAL);
> + rcu_read_unlock();
> + if (is_scoped)
> + return -EPERM;
> + return 0;
> +}
> +
> static struct security_hook_list landlock_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
> LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
> LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
> LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
> LSM_HOOK_INIT(task_kill, hook_task_kill),
> + LSM_HOOK_INIT(file_send_sigiotask, hook_file_send_sigiotask),
> };
>
> __init void landlock_add_task_hooks(void)
> --
> 2.34.1
>
Powered by blists - more mailing lists