| Message ID | d04bc943e8d275e8d00bb7742bcdbabc7913abbe.1723680305.git.fahimitahera@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: Signal Scoping Support | expand |
On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.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@gmail.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 >
On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote: > On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.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@gmail.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. Hi Jann, Thanks for the feedback :) It totally make sense. > > + 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); I was thinking of if we can replace this landlock_put_ruleset with landlock_put_ruleset_deferred. In this case, it would be better use of handling the lock? > > + 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. Correct, I applied the changes. > > + 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 > >
On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi <fahimitahera@gmail.com> wrote: > > On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote: > > On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.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@gmail.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. > Hi Jann, > > Thanks for the feedback :) > It totally make sense. > > > + 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); > I was thinking of if we can replace this landlock_put_ruleset with > landlock_put_ruleset_deferred. In this case, it would be better use of > handling the lock? I don't think you have to take the "file->f_owner.lock" in this hook - the file has already been torn down pretty far, nothing is going to be able to trigger the file_set_fowner hook anymore. But either way, you're right that we can't just use landlock_put_ruleset() here because landlock_put_ruleset() can sleep and the file_free_security hook can be invoked from non-sleepable context. (This only happens when fput() directly calls file_free(), and I think that only happens with ->fown_domain==NULL, so technically it would also be fine to do something like "if (domain) landlock_put_ruleset(domain);".) If you test your current code in a kernel that was built with CONFIG_DEBUG_ATOMIC_SLEEP=y, this will probably print an warning message in the kernel log (dmesg). You're right that using landlock_put_ruleset_deferred() instead would fix that. I think the right solution here is probably just to do: static void hook_file_free_security(struct file *file) { landlock_put_ruleset_deferred(landlock_file(file)->fown_domain); } Alternatively it would also work to do this - this code is probably a bit more efficient but also a little less clear: static void hook_file_free_security(struct file *file) { /* don't trigger might_sleep() for tearing down unopened file */ if (landlock_file(file)->fown_domain) landlock_put_ruleset(landlock_file(file)->fown_domain); } > > > > + write_unlock_irq(&file->f_owner.lock); > > > +}
On Fri, Aug 16, 2024 at 12:10:44AM +0200, Jann Horn wrote: > On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi <fahimitahera@gmail.com> wrote: > > > > On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote: > > > On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.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@gmail.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. > > Hi Jann, > > > > Thanks for the feedback :) > > It totally make sense. > > > > + 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); > > I was thinking of if we can replace this landlock_put_ruleset with > > landlock_put_ruleset_deferred. In this case, it would be better use of > > handling the lock? > > I don't think you have to take the "file->f_owner.lock" in this hook - > the file has already been torn down pretty far, nothing is going to be > able to trigger the file_set_fowner hook anymore. That's right. Thanks. > But either way, you're right that we can't just use > landlock_put_ruleset() here because landlock_put_ruleset() can sleep > and the file_free_security hook can be invoked from non-sleepable > context. (This only happens when fput() directly calls file_free(), > and I think that only happens with ->fown_domain==NULL, so technically > it would also be fine to do something like "if (domain) > landlock_put_ruleset(domain);".) > If you test your current code in a kernel that was built with > CONFIG_DEBUG_ATOMIC_SLEEP=y, this will probably print an warning > message in the kernel log (dmesg). You're right that using > landlock_put_ruleset_deferred() instead would fix that. > > I think the right solution here is probably just to do: > > static void hook_file_free_security(struct file *file) > { > landlock_put_ruleset_deferred(landlock_file(file)->fown_domain); > } I think I will stick to this one since it is easier to understand. > Alternatively it would also work to do this - this code is probably a > bit more efficient but also a little less clear: > > static void hook_file_free_security(struct file *file) > { > /* don't trigger might_sleep() for tearing down unopened file */ > if (landlock_file(file)->fown_domain) > landlock_put_ruleset(landlock_file(file)->fown_domain); > } > > > > > > > + write_unlock_irq(&file->f_owner.lock); > > > > +}
On Thu, Aug 15, 2024 at 12:29:21PM -0600, Tahera Fahimi 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. We need to make sure this file_send_sigiotask hook is useful and can be triggered by user space code. Currently, all tests pass without this patch.
On Thu, Aug 15, 2024 at 12:29:21PM -0600, Tahera Fahimi 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@gmail.com> > --- > security/landlock/fs.c | 18 ++++++++++++++++++ > security/landlock/fs.h | 6 ++++++ > security/landlock/task.c | 27 +++++++++++++++++++++++++++ > 3 files changed, 51 insertions(+) Please squash this patch with the previous one, both are enforcing the signal scoping restriction with LANDLOCK_SCOPED_SIGNAL. You'll also need to update the scoped_test.c file with LANDLOCK_SCOPED_SIGNAL (in this same squashed patch).
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); + 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); + 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)
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@gmail.com> --- security/landlock/fs.c | 18 ++++++++++++++++++ security/landlock/fs.h | 6 ++++++ security/landlock/task.c | 27 +++++++++++++++++++++++++++ 3 files changed, 51 insertions(+)