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

[Paul Moore <paul@paul-moore.com>]

On Thu, Aug 31, 2023 at 11:13 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> 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.

Yes, thanks all.  I figured something was a little wonky but wasn't
smart enough to know the correct fix.

So .... who wants to submit a patch?

-- 
paul-moore.com