Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

On Fri, Aug 16, 2024 at 12:10:44AM +0200, Jann Horn wrote:
> On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > >  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);
> > > > +}