diff mbox series

[21/28] security: Introduce inode_post_remove_acl hook

Message ID	20230303181842.1087717-22-roberto.sassu@huaweicloud.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: viro@zeniv.linux.org.uk, chuck.lever@oracle.com, jlayton@kernel.org, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, dhowells@redhat.com, jarkko@kernel.org, stephen.smalley.work@gmail.com, eparis@parisplace.org, casey@schaufler-ca.com, brauner@kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, selinux@vger.kernel.org, linux-kernel@vger.kernel.org, stefanb@linux.ibm.com, Roberto Sassu <roberto.sassu@huawei.com> Subject: [PATCH 21/28] security: Introduce inode_post_remove_acl hook Date: Fri, 3 Mar 2023 19:18:35 +0100 Message-Id: <20230303181842.1087717-22-roberto.sassu@huaweicloud.com> In-Reply-To: <20230303181842.1087717-1-roberto.sassu@huaweicloud.com> References: <20230303181842.1087717-1-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	security: Move IMA and EVM to the LSM infrastructure \| [00/28] security: Move IMA and EVM to the LSM infrastructure [01/28] ima: Align ima_inode_post_setattr() definition with LSM infrastructure [02/28] ima: Align ima_post_path_mknod() definition with LSM infrastructure [03/28] ima: Align ima_post_create_tmpfile() definition with LSM infrastructure [04/28] ima: Align ima_file_mprotect() definition with LSM infrastructure [05/28] ima: Align ima_inode_setxattr() definition with LSM infrastructure [06/28] ima: Align ima_inode_removexattr() definition with LSM infrastructure [07/28] ima: Align ima_post_read_file() definition with LSM infrastructure [08/28] evm: Align evm_inode_post_setattr() definition with LSM infrastructure [09/28] evm: Align evm_inode_setxattr() definition with LSM infrastructure [10/28] evm: Align evm_inode_post_setxattr() definition with LSM infrastructure [11/28] evm: Complete description of evm_inode_setattr() [12/28] fs: Fix description of vfs_tmpfile() [13/28] security: Align inode_setattr hook definition with EVM [14/28] security: Introduce inode_post_setattr hook [15/28] security: Introduce inode_post_removexattr hook [16/28] security: Introduce file_post_open hook [17/28] security: Introduce file_pre_free_security hook [18/28] security: Introduce path_post_mknod hook [19/28] security: Introduce inode_post_create_tmpfile hook [20/28] security: Introduce inode_post_set_acl hook [21/28] security: Introduce inode_post_remove_acl hook [22/28] security: Introduce key_post_create_or_update hook [23/28] security: Introduce LSM_ORDER_LAST [24/28] ima: Move to LSM infrastructure [25/28] ima: Move IMA-Appraisal to LSM infrastructure [26/28] evm: Move to LSM infrastructure [27/28] integrity: Move integrity functions to the LSM infrastructure [28/28] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache

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