Re: [RFC PATCH] lsm: add the inode_free_security_rcu() LSM implementation hook
From: Dave Chinner <david@fromorbit.com>
Date: 2024-07-23 03:34:48
Also in:
linux-fsdevel, selinux
On Mon, Jul 22, 2024 at 03:46:36PM -0400, Paul Moore wrote:
On Mon, Jul 22, 2024 at 8:30 AM Matus Jokay [off-list ref] wrote:quoted
On 10. 7. 2024 12:40, Mickaël Salaün wrote:quoted
On Tue, Jul 09, 2024 at 10:40:30PM -0400, Paul Moore wrote:quoted
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@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.quoted
--- 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(-)...quoted
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. -- Dave Chinner david@fromorbit.com