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@xxxxxxxx> 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@xxxxxxxxxxxxxx> > > > > > > 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. -- Dave Chinner david@xxxxxxxxxxxxx
