[RFC PATCH] lsm: add the inode_free_security_rcu() LSM implementation hook

[Paul Moore]

On Mon, Jul 22, 2024 at 8:30 AM Matus Jokay <matus.jokay at stuba.sk> wrote:
> On 10. 7. 2024 12:40, Mickaël Salaün wrote:
> > On Tue, Jul 09, 2024 at 10:40:30PM -0400, Paul Moore wrote:
> >> The LSM framework has an existing inode_free_security() hook which
> >> is used by LSMs that manage state associated with an inode, but
> >> due to the use of RCU to protect the inode, special care must be
> >> taken to ensure that the LSMs do not fully release the inode state
> >> until it is safe from a RCU perspective.
> >>
> >> This patch implements a new inode_free_security_rcu() implementation
> >> hook which is called when it is safe to free the LSM's internal inode
> >> state.  Unfortunately, this new hook does not have access to the inode
> >> itself as it may already be released, so the existing
> >> inode_free_security() hook is retained for those LSMs which require
> >> access to the inode.
> >>
> >> Signed-off-by: Paul Moore <paul at paul-moore.com>
> >
> > I like this new hook.  It is definitely safer than the current approach.
> >
> > To make it more consistent, I think we should also rename
> > security_inode_free() to security_inode_put() to highlight the fact that
> > LSM implementations should not free potential pointers in this blob
> > because they could still be dereferenced in a path walk.
> >
> >> ---
> >>  include/linux/lsm_hook_defs.h     |  1 +
> >>  security/integrity/ima/ima.h      |  2 +-
> >>  security/integrity/ima/ima_iint.c | 20 ++++++++------------
> >>  security/integrity/ima/ima_main.c |  2 +-
> >>  security/landlock/fs.c            |  9 ++++++---
> >>  security/security.c               | 26 +++++++++++++-------------
> >>  6 files changed, 30 insertions(+), 30 deletions(-)

...

> Sorry for the questions, but for several weeks I can't find answers to two things related to this RFC:
>
> 1) How does this patch close [1]?
>    As Mickaël pointed in [2], "It looks like security_inode_free() is called two times on the same inode."
>    Indeed, it does not seem from the backtrace that it is a case of race between destroy_inode and inode_permission,
>    i.e. referencing the inode in a VFS path walk while destroying it...
>    Please, can anyone tell me how this situation could have happened? Maybe folks from VFS... I added them to the copy.

The VFS folks can likely provide a better, or perhaps a more correct
answer, but my understanding is that during the path walk the inode is
protected by a RCU lock which allows for multiple threads to access
the inode simultaneously; this could result in some cases where one
thread is destroying the inode while another is accessing it.
Changing this would require changes to the VFS code, and I'm not sure
why you would want to change it anyway, the performance win of using
RCU here is likely significant.

> 2) Is there a guarantee that inode_free_by_rcu and i_callback will be called within the same RCU grace period?

I'm not an RCU expert, but I don't believe there are any guarantees
that the inode_free_by_rcu() and the inode's own free routines are
going to be called within the same RCU grace period (not really
applicable as inode_free_by_rcu() isn't called *during* a grace
period, but *after* the grace period of the associated
security_inode_free() call).  However, this patch does not rely on
synchronization between the inode and inode LSM free routine in
inode_free_by_rcu(); the inode_free_by_rcu() function and the new
inode_free_security_rcu() LSM callback does not have a pointer to the
inode, only the inode's LSM blob.  I agree that it is a bit odd, but
freeing the inode and inode's LSM blob independently of each other
should not cause a problem so long as the inode is no longer in use
(hence the RCU callbacks).

>    If not, can the security context be released earlier than the inode itself?

Possibly, but it should only happen after the inode is no longer in
use (the call_rcu () callback should ensure that we are past all of
the currently executing RCU critical sections).

> If yes, can be executed
>    inode_permission concurrently, leading to UAF of inode security context in security_inode_permission?

I do not believe so, see the discussion above, but I welcome any corrections.

>    Can fsnotify affect this (leading to different RCU grace periods)? (Again probably a question for VFS people.)

If fsnotify is affecting this negatively then I suspect that is a
reason for much larger concern as I believe that would indicate a
problem with fsnotify and the inode locking scheme.

-- 
paul-moore.com