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:

quoted

On Wed, 30 Aug 2023, Paul Moore wrote:

quoted

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.