diff mbox series

[v3,12/25] security: Introduce inode_post_setattr hook

Message ID 20230904133415.1799503-13-roberto.sassu@huaweicloud.com (mailing list archive)
State New
Headers show
Series security: Move IMA and EVM to the LSM infrastructure | expand

Commit Message

Roberto Sassu Sept. 4, 2023, 1:34 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_setattr hook.

It is useful for EVM to recalculate the HMAC on modified file attributes
and other file metadata, after it verified the HMAC of current file
metadata with the inode_setattr hook.

LSMs should use the new hook instead of inode_setattr, when they need to
know that the operation was done successfully (not known in inode_setattr).
The new hook cannot return an error and cannot cause the operation to be
reverted.

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

Comments

Stefan Berger Sept. 5, 2023, 5:40 p.m. UTC | #1
On 9/4/23 09:34, 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_setattr hook.
>
> It is useful for EVM to recalculate the HMAC on modified file attributes
> and other file metadata, after it verified the HMAC of current file
> metadata with the inode_setattr hook.
>
> LSMs should use the new hook instead of inode_setattr, when they need to
> know that the operation was done successfully (not known in inode_setattr).
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   fs/attr.c                     |  1 +
>   include/linux/lsm_hook_defs.h |  2 ++
>   include/linux/security.h      |  7 +++++++
>   security/security.c           | 16 ++++++++++++++++
>   4 files changed, 26 insertions(+)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 431f667726c7..3c309eb456c6 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -486,6 +486,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>   
>   	if (!error) {
>   		fsnotify_change(dentry, ia_valid);
> +		security_inode_post_setattr(idmap, dentry, ia_valid);
>   		ima_inode_post_setattr(idmap, dentry, ia_valid);
>   		evm_inode_post_setattr(idmap, dentry, ia_valid);
>   	}
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index fdf075a6b1bb..995d30336cfa 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -136,6 +136,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
>   LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
>   LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
>   	 struct iattr *attr)
> +LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
> +	 struct dentry *dentry, int ia_valid)
>   LSM_HOOK(int, 0, inode_getattr, const struct path *path)
>   LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
>   	 struct dentry *dentry, const char *name, const void *value,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dcb3604ffab8..820899db5276 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -355,6 +355,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
>   int security_inode_permission(struct inode *inode, int mask);
>   int security_inode_setattr(struct mnt_idmap *idmap,
>   			   struct dentry *dentry, struct iattr *attr);
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +				 int ia_valid);
>   int security_inode_getattr(const struct path *path);
>   int security_inode_setxattr(struct mnt_idmap *idmap,
>   			    struct dentry *dentry, const char *name,
> @@ -856,6 +858,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap,
>   	return 0;
>   }
>   
> +static inline void
> +security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +			    int ia_valid)
> +{ }

Existing security_sem_free() and others are also formatted like it, so 
it should be ok.


> +
>   static inline int security_inode_getattr(const struct path *path)
>   {
>   	return 0;
> diff --git a/security/security.c b/security/security.c
> index 2b24d01cf181..764a6f28b3b9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2124,6 +2124,22 @@ int security_inode_setattr(struct mnt_idmap *idmap,
>   }
>   EXPORT_SYMBOL_GPL(security_inode_setattr);
>   
> +/**
> + * security_inode_post_setattr() - Update the inode after a setattr operation
> + * @idmap: idmap of the mount
> + * @dentry: file
> + * @ia_valid: file attributes set
> + *
> + * Update inode security field after successful setting file attributes.
> + */
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +				 int ia_valid)
> +{
> +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> +		return;
> +	call_void_hook(inode_post_setattr, idmap, dentry, ia_valid);
> +}
> +
>   /**
>    * security_inode_getattr() - Check if getting file attributes is allowed
>    * @path: file

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Roberto Sassu Sept. 26, 2023, 11:14 a.m. UTC | #2
On Mon, 2023-09-04 at 15:34 +0200, 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_setattr hook.
> 
> It is useful for EVM to recalculate the HMAC on modified file attributes
> and other file metadata, after it verified the HMAC of current file
> metadata with the inode_setattr hook.
> 
> LSMs should use the new hook instead of inode_setattr, when they need to
> know that the operation was done successfully (not known in inode_setattr).
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/attr.c                     |  1 +

Hi Christian, Al

could you please review and ack patches 12-19, which touch the VFS?

Thanks a lot!

Roberto

>  include/linux/lsm_hook_defs.h |  2 ++
>  include/linux/security.h      |  7 +++++++
>  security/security.c           | 16 ++++++++++++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 431f667726c7..3c309eb456c6 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -486,6 +486,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>  
>  	if (!error) {
>  		fsnotify_change(dentry, ia_valid);
> +		security_inode_post_setattr(idmap, dentry, ia_valid);
>  		ima_inode_post_setattr(idmap, dentry, ia_valid);
>  		evm_inode_post_setattr(idmap, dentry, ia_valid);
>  	}
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index fdf075a6b1bb..995d30336cfa 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -136,6 +136,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
>  LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
>  LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
>  	 struct iattr *attr)
> +LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
> +	 struct dentry *dentry, int ia_valid)
>  LSM_HOOK(int, 0, inode_getattr, const struct path *path)
>  LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
>  	 struct dentry *dentry, const char *name, const void *value,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dcb3604ffab8..820899db5276 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -355,6 +355,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
>  int security_inode_permission(struct inode *inode, int mask);
>  int security_inode_setattr(struct mnt_idmap *idmap,
>  			   struct dentry *dentry, struct iattr *attr);
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +				 int ia_valid);
>  int security_inode_getattr(const struct path *path);
>  int security_inode_setxattr(struct mnt_idmap *idmap,
>  			    struct dentry *dentry, const char *name,
> @@ -856,6 +858,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap,
>  	return 0;
>  }
>  
> +static inline void
> +security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +			    int ia_valid)
> +{ }
> +
>  static inline int security_inode_getattr(const struct path *path)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 2b24d01cf181..764a6f28b3b9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2124,6 +2124,22 @@ int security_inode_setattr(struct mnt_idmap *idmap,
>  }
>  EXPORT_SYMBOL_GPL(security_inode_setattr);
>  
> +/**
> + * security_inode_post_setattr() - Update the inode after a setattr operation
> + * @idmap: idmap of the mount
> + * @dentry: file
> + * @ia_valid: file attributes set
> + *
> + * Update inode security field after successful setting file attributes.
> + */
> +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> +				 int ia_valid)
> +{
> +	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> +		return;
> +	call_void_hook(inode_post_setattr, idmap, dentry, ia_valid);
> +}
> +
>  /**
>   * security_inode_getattr() - Check if getting file attributes is allowed
>   * @path: file
Mimi Zohar Oct. 12, 2023, 12:08 a.m. UTC | #3
gOn Mon, 2023-09-04 at 15:34 +0200, 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_setattr hook.
> 
> It is useful for EVM to recalculate the HMAC on modified file attributes
> and other file metadata, after it verified the HMAC of current file
> metadata with the inode_setattr hook.

"useful"?  

At inode_setattr hook, EVM verifies the file's existing HMAC value.  At
inode_post_setattr, EVM re-calculates the file's HMAC based on the
modified file attributes and other file metadata.

> 
> LSMs should use the new hook instead of inode_setattr, when they need to
> know that the operation was done successfully (not known in inode_setattr).
> The new hook cannot return an error and cannot cause the operation to be
> reverted.

Other LSMs could similarly update security xattrs or ...

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Roberto Sassu Oct. 12, 2023, 7:42 a.m. UTC | #4
On Wed, 2023-10-11 at 20:08 -0400, Mimi Zohar wrote:
> gOn Mon, 2023-09-04 at 15:34 +0200, 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_setattr hook.
> > 
> > It is useful for EVM to recalculate the HMAC on modified file attributes
> > and other file metadata, after it verified the HMAC of current file
> > metadata with the inode_setattr hook.
> 
> "useful"?  
> 
> At inode_setattr hook, EVM verifies the file's existing HMAC value.  At
> inode_post_setattr, EVM re-calculates the file's HMAC based on the
> modified file attributes and other file metadata.
> 
> > 
> > LSMs should use the new hook instead of inode_setattr, when they need to
> > know that the operation was done successfully (not known in inode_setattr).
> > The new hook cannot return an error and cannot cause the operation to be
> > reverted.
> 
> Other LSMs could similarly update security xattrs or ...

I added your sentence. The one above is to satisfy Casey's request to
justify the addition of the new hook, and to explain why inode_setattr
is not sufficient.

Thanks

Roberto

> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Mimi Zohar Oct. 12, 2023, 11:43 a.m. UTC | #5
On Thu, 2023-10-12 at 09:42 +0200, Roberto Sassu wrote:
> On Wed, 2023-10-11 at 20:08 -0400, Mimi Zohar wrote:
> > gOn Mon, 2023-09-04 at 15:34 +0200, 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_setattr hook.
> > > 
> > > It is useful for EVM to recalculate the HMAC on modified file attributes
> > > and other file metadata, after it verified the HMAC of current file
> > > metadata with the inode_setattr hook.
> > 
> > "useful"?  
> > 
> > At inode_setattr hook, EVM verifies the file's existing HMAC value.  At
> > inode_post_setattr, EVM re-calculates the file's HMAC based on the
> > modified file attributes and other file metadata.
> > 
> > > 
> > > LSMs should use the new hook instead of inode_setattr, when they need to
> > > know that the operation was done successfully (not known in inode_setattr).
> > > The new hook cannot return an error and cannot cause the operation to be
> > > reverted.
> > 
> > Other LSMs could similarly update security xattrs or ...
> 
> I added your sentence. The one above is to satisfy Casey's request to
> justify the addition of the new hook, and to explain why inode_setattr
> is not sufficient.

I was suggesting simplifying the wording.  Perhaps something like:

Other LSMs could similarly take some action after successful file attri
bute change.
Roberto Sassu Oct. 12, 2023, 12:25 p.m. UTC | #6
On Thu, 2023-10-12 at 07:43 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-12 at 09:42 +0200, Roberto Sassu wrote:
> > On Wed, 2023-10-11 at 20:08 -0400, Mimi Zohar wrote:
> > > gOn Mon, 2023-09-04 at 15:34 +0200, 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_setattr hook.
> > > > 
> > > > It is useful for EVM to recalculate the HMAC on modified file attributes
> > > > and other file metadata, after it verified the HMAC of current file
> > > > metadata with the inode_setattr hook.
> > > 
> > > "useful"?  
> > > 
> > > At inode_setattr hook, EVM verifies the file's existing HMAC value.  At
> > > inode_post_setattr, EVM re-calculates the file's HMAC based on the
> > > modified file attributes and other file metadata.
> > > 
> > > > 
> > > > LSMs should use the new hook instead of inode_setattr, when they need to
> > > > know that the operation was done successfully (not known in inode_setattr).
> > > > The new hook cannot return an error and cannot cause the operation to be
> > > > reverted.
> > > 
> > > Other LSMs could similarly update security xattrs or ...
> > 
> > I added your sentence. The one above is to satisfy Casey's request to
> > justify the addition of the new hook, and to explain why inode_setattr
> > is not sufficient.
> 
> I was suggesting simplifying the wording.  Perhaps something like:
> 
> Other LSMs could similarly take some action after successful file attri
> bute change.

Ok, will use that.

Thanks

Roberto
diff mbox series

Patch

diff --git a/fs/attr.c b/fs/attr.c
index 431f667726c7..3c309eb456c6 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -486,6 +486,7 @@  int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
 
 	if (!error) {
 		fsnotify_change(dentry, ia_valid);
+		security_inode_post_setattr(idmap, dentry, ia_valid);
 		ima_inode_post_setattr(idmap, dentry, ia_valid);
 		evm_inode_post_setattr(idmap, dentry, ia_valid);
 	}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index fdf075a6b1bb..995d30336cfa 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -136,6 +136,8 @@  LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
 LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
 LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry,
 	 struct iattr *attr)
+LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap,
+	 struct dentry *dentry, int ia_valid)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
 LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name, const void *value,
diff --git a/include/linux/security.h b/include/linux/security.h
index dcb3604ffab8..820899db5276 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -355,6 +355,8 @@  int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
 int security_inode_permission(struct inode *inode, int mask);
 int security_inode_setattr(struct mnt_idmap *idmap,
 			   struct dentry *dentry, struct iattr *attr);
+void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+				 int ia_valid);
 int security_inode_getattr(const struct path *path);
 int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
@@ -856,6 +858,11 @@  static inline int security_inode_setattr(struct mnt_idmap *idmap,
 	return 0;
 }
 
+static inline void
+security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+			    int ia_valid)
+{ }
+
 static inline int security_inode_getattr(const struct path *path)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 2b24d01cf181..764a6f28b3b9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2124,6 +2124,22 @@  int security_inode_setattr(struct mnt_idmap *idmap,
 }
 EXPORT_SYMBOL_GPL(security_inode_setattr);
 
+/**
+ * security_inode_post_setattr() - Update the inode after a setattr operation
+ * @idmap: idmap of the mount
+ * @dentry: file
+ * @ia_valid: file attributes set
+ *
+ * Update inode security field after successful setting file attributes.
+ */
+void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+				 int ia_valid)
+{
+	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+		return;
+	call_void_hook(inode_post_setattr, idmap, dentry, ia_valid);
+}
+
 /**
  * security_inode_getattr() - Check if getting file attributes is allowed
  * @path: file