On Thu, Aug 31, 2023 at 11:13 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> 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
