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

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

[Paul Moore]

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.

-- 
paul-moore.com

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

[Hugh Dickins]

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.

Anyway, I think it's a question better answered by Mimi and Al.

Thanks,
Hugh

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

[Christian Brauner]

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()

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

[Mimi Zohar]

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.

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

[Paul Moore]

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