diff mbox series

[21/28] security: Introduce inode_post_remove_acl hook

Message ID 20230303181842.1087717-22-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series security: Move IMA and EVM to the LSM infrastructure | expand

Commit Message

Roberto Sassu March 3, 2023, 6:18 p.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the inode_post_remove_acl hook.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/posix_acl.c                |  1 +
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/security.h      |  8 ++++++++
 security/security.c           | 17 +++++++++++++++++
 4 files changed, 28 insertions(+)

Comments

Stefan Berger March 6, 2023, 3:22 p.m. UTC | #1 
On 3/3/23 13:18, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the inode_post_remove_acl hook.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

>   
> +/**
> + * security_inode_post_remove_acl() - Update inode sec after remove_acl op
> + * @idmap: idmap of the mount
> + * @dentry: file
> + * @acl_name: acl name
> + *
> + * Update inode security field after successful remove_acl operation on @dentry
> + * in @idmap. The posix acls are identified by @acl_name.
> + */
> +void security_inode_post_remove_acl(struct mnt_idmap *idmap,
> +				    struct dentry *dentry, const char *acl_name)
> +{
> +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> +		return;

Was that a mistake before that EVM and IMA functions did not filtered out private inodes?

    Stefan
Roberto Sassu March 6, 2023, 3:34 p.m. UTC | #2 
On Mon, 2023-03-06 at 10:22 -0500, Stefan Berger wrote:
> 
> On 3/3/23 13:18, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > the inode_post_remove_acl hook.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   
> > +/**
> > + * security_inode_post_remove_acl() - Update inode sec after remove_acl op
> > + * @idmap: idmap of the mount
> > + * @dentry: file
> > + * @acl_name: acl name
> > + *
> > + * Update inode security field after successful remove_acl operation on @dentry
> > + * in @idmap. The posix acls are identified by @acl_name.
> > + */
> > +void security_inode_post_remove_acl(struct mnt_idmap *idmap,
> > +				    struct dentry *dentry, const char *acl_name)
> > +{
> > +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > +		return;
> 
> Was that a mistake before that EVM and IMA functions did not filtered out private inodes?

Looks like that. At least for hooks that are not called from
security.c.

Thanks

Roberto
Stefan Berger March 6, 2023, 4:16 p.m. UTC | #3 
On 3/6/23 10:34, Roberto Sassu wrote:
> On Mon, 2023-03-06 at 10:22 -0500, Stefan Berger wrote:
>>
>> On 3/3/23 13:18, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>
>>> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
>>> the inode_post_remove_acl hook.
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>>    
>>> +/**
>>> + * security_inode_post_remove_acl() - Update inode sec after remove_acl op
>>> + * @idmap: idmap of the mount
>>> + * @dentry: file
>>> + * @acl_name: acl name
>>> + *
>>> + * Update inode security field after successful remove_acl operation on @dentry
>>> + * in @idmap. The posix acls are identified by @acl_name.
>>> + */
>>> +void security_inode_post_remove_acl(struct mnt_idmap *idmap,
>>> +				    struct dentry *dentry, const char *acl_name)
>>> +{
>>> +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>>> +		return;
>>
>> Was that a mistake before that EVM and IMA functions did not filtered out private inodes?
> 
> Looks like that. At least for hooks that are not called from
> security.c.

It seems like that all security_* functions are filtering on private inodes. Anonymous inodes have them and some filesystem set the S_PRIVATE flag. So it may not make a difference fro IMA and EVM then.

     Stefan

> 
> Thanks
> 
> Roberto
>
Roberto Sassu March 6, 2023, 4:50 p.m. UTC | #4 
On Mon, 2023-03-06 at 11:16 -0500, Stefan Berger wrote:
> 
> On 3/6/23 10:34, Roberto Sassu wrote:
> > On Mon, 2023-03-06 at 10:22 -0500, Stefan Berger wrote:
> > > On 3/3/23 13:18, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > > > the inode_post_remove_acl hook.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >    
> > > > +/**
> > > > + * security_inode_post_remove_acl() - Update inode sec after remove_acl op
> > > > + * @idmap: idmap of the mount
> > > > + * @dentry: file
> > > > + * @acl_name: acl name
> > > > + *
> > > > + * Update inode security field after successful remove_acl operation on @dentry
> > > > + * in @idmap. The posix acls are identified by @acl_name.
> > > > + */
> > > > +void security_inode_post_remove_acl(struct mnt_idmap *idmap,
> > > > +				    struct dentry *dentry, const char *acl_name)
> > > > +{
> > > > +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > > > +		return;
> > > 
> > > Was that a mistake before that EVM and IMA functions did not filtered out private inodes?
> > 
> > Looks like that. At least for hooks that are not called from
> > security.c.
> 
> It seems like that all security_* functions are filtering on private inodes. Anonymous inodes have them and some filesystem set the S_PRIVATE flag. So it may not make a difference fro IMA and EVM then.

Currently, what it would happen is that the HMAC would be updated
without check, since the check function is usually in security.c
(skipped) and the post elsewhere.

With this patch set, also the post function would not be executed for
private inodes. Maybe, it is worth mentioning it in the next version.

Thanks

Roberto
diff mbox series

Patch

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index acddf2dff4c..5b8c92fce0c 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -1213,6 +1213,7 @@  int vfs_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 		error = -EOPNOTSUPP;
 	if (!error) {
 		fsnotify_xattr(dentry);
+		security_inode_post_remove_acl(idmap, dentry, acl_name);
 		evm_inode_post_remove_acl(idmap, dentry, acl_name);
 	}
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 9a3e14db0af..6c324fe5099 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -162,6 +162,8 @@  LSM_HOOK(int, 0, inode_get_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name)
 LSM_HOOK(int, 0, inode_remove_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_remove_acl, struct mnt_idmap *idmap,
+	 struct dentry *dentry, const char *acl_name)
 LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
 LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
 	 struct dentry *dentry)
diff --git a/include/linux/security.h b/include/linux/security.h
index b0691bf7237..f8df5b69667 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -372,6 +372,9 @@  int security_inode_get_acl(struct mnt_idmap *idmap,
 			   struct dentry *dentry, const char *acl_name);
 int security_inode_remove_acl(struct mnt_idmap *idmap,
 			      struct dentry *dentry, const char *acl_name);
+void security_inode_post_remove_acl(struct mnt_idmap *idmap,
+				    struct dentry *dentry,
+				    const char *acl_name);
 void security_inode_post_setxattr(struct dentry *dentry, const char *name,
 				  const void *value, size_t size, int flags);
 int security_inode_getxattr(struct dentry *dentry, const char *name);
@@ -914,6 +917,11 @@  static inline int security_inode_remove_acl(struct mnt_idmap *idmap,
 	return 0;
 }
 
+static inline void security_inode_post_remove_acl(struct mnt_idmap *idmap,
+						  struct dentry *dentry,
+						  const char *acl_name)
+{ }
+
 static inline void security_inode_post_setxattr(struct dentry *dentry,
 		const char *name, const void *value, size_t size, int flags)
 { }
diff --git a/security/security.c b/security/security.c
index fc11d70bb02..b3a9c317f75 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2373,6 +2373,23 @@  int security_inode_remove_acl(struct mnt_idmap *idmap,
 	return evm_inode_remove_acl(idmap, dentry, acl_name);
 }
 
+/**
+ * security_inode_post_remove_acl() - Update inode sec after remove_acl op
+ * @idmap: idmap of the mount
+ * @dentry: file
+ * @acl_name: acl name
+ *
+ * Update inode security field after successful remove_acl operation on @dentry
+ * in @idmap. The posix acls are identified by @acl_name.
+ */
+void security_inode_post_remove_acl(struct mnt_idmap *idmap,
+				    struct dentry *dentry, const char *acl_name)
+{
+	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+		return;
+	call_void_hook(inode_post_remove_acl, idmap, dentry, acl_name);
+}
+
 /**
  * security_inode_post_setxattr() - Update the inode after a setxattr operation
  * @dentry: file