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

[Matus Jokay]

On 23. 7. 2024 5:34, Dave Chinner wrote:
> On Mon, Jul 22, 2024 at 03:46:36PM -0400, Paul Moore wrote:
>> 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.
> 
> Shouldn't may_lookup() be checking the inode for (I_NEW |
> I_WILLFREE | I_FREE) so that it doesn't access an inode either not
> completely initialised or being evicted during the RCU path walk?
> All accesses to the VFS inode that don't have explicit reference
> counts have to do these checks...
> 
> IIUC, at the may_lookup() point, the RCU pathwalk doesn't have a
> fully validate reference count to the dentry or the inode at this
> point, so it seems accessing random objects attached to an inode
> that can be anywhere in the setup or teardown process isn't at all
> safe...
> 
> -Dave.

Indeed, but maybe only VFS maintainers can give us the answer to why may_lookup()
does not need at some point check for (I_NEW | I_WILL_FREE | I_FREEING).

Thanks,
mY