On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi [off-list ref] wrote:

On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote:

quoted

On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi [off-list ref] wrote:

quoted

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 <redacted>
---
 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.

quoted

quoted

+       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);
}

quoted

quoted

+       write_unlock_irq(&file->f_owner.lock);
+}