Re: LSM hook ordering in shmem_mknod() and shmem_tmpfile()?

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2023-08-31 at 14:36 +0200, Christian Brauner wrote:
> On Thu, Aug 31, 2023 at 02:19:20AM -0700, Hugh Dickins wrote:
> > On Wed, 30 Aug 2023, Paul Moore wrote:
> > 
> > > Hello all,
> > > 
> > > While looking at some recent changes in mm/shmem.c I noticed that the
> > > ordering between simple_acl_create() and
> > > security_inode_init_security() is different between shmem_mknod() and
> > > shmem_tmpfile().  In shmem_mknod() the ACL call comes before the LSM
> > > hook, and in shmem_tmpfile() the LSM call comes before the ACL call.
> > > 
> > > Perhaps this is correct, but it seemed a little odd to me so I wanted
> > > to check with all of you to make sure there is a good reason for the
> > > difference between the two functions.  Looking back to when
> > > shmem_tmpfile() was created ~2013 I don't see any explicit mention as
> > > to why the ordering is different so I'm looking for a bit of a sanity
> > > check to see if I'm missing something obvious.
> > > 
> > > My initial thinking this morning is that the
> > > security_inode_init_security() call should come before
> > > simple_acl_create() in both cases, but I'm open to different opinions
> > > on this.
> > 
> > Good eye.  The crucial commit here appears to be Mimi's 3.11 commit
> > 37ec43cdc4c7 "evm: calculate HMAC after initializing posix acl on tmpfs"
> > which intentionally moved shmem_mknod()'s generic_acl_init() up before
> > the security_inode_init_security(), around the same time as Al was
> > copying shmem_mknod() to introduce shmem_tmpfile().
> > 
> > I'd have agreed with you, Paul, until reading Mimi's commit:
> > now it looks more like shmem_tmpfile() is the one to be changed,
> > except (I'm out of my depth) maybe it's irrelevant on tmpfiles.
> 
> POSIX ACLs generally need to be set first as they are may change inode
> properties that security_inode_init_security() may rely on to be stable.
> That specifically incudes inode->i_mode:
> 
> * If the filesystem doesn't support POSIX ACLs then the umask is
>   stripped in the VFS before it ever gets to the filesystems. For such
>   cases the order of *_init_security() and setting POSIX ACLs doesn't
>   matter.
> * If the filesystem does support POSIX ACLs and the directory of the
>   resulting file does have default POSIX ACLs with mode settings then
>   the inode->i_mode will be updated.
> * If the filesystem does support POSIX ACLs but the directory doesn't
>   have default POSIX ACLs the umask will be stripped.
> 
> (roughly from memory)
> 
> If tmpfs is compiled with POSIX ACL support the mode might change and if
> anything in *_init_security() relies on inode->i_mode being stable it
> needs to be called after they have been set.
> 
> EVM hashes do use the mode and the hash gets updated when POSIX ACLs are
> changed - which caused me immense pain when I redid these codepaths last
> year.
> 
> IMHO, the easiest fix really is to lump all this together for all
> creation paths. This is what most filesystems do. For examples, see
> 
> xfs_generic_create()
> -> posix_acl_create(&mode)
> -> xfs_create{_tmpfile}(mode)
> -> xfs_inode_init_security()
> 
> or
> 
> __ext4_new_inode()
> -> ext4_init_acl()
> -> ext4_init_security()

Agreed.  Thanks, Hugh, Christian for the clear explanation.