diff mbox series

[v5,13/23] security: Introduce file_pre_free_security hook

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

Commit Message

Roberto Sassu Nov. 7, 2023, 1:40 p.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com>

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

IMA calculates at file close the new digest of the file content and writes
it to security.ima, so that appraisal at next file access succeeds.

LSMs could also take some action before the last reference of a file is
released.

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/file_table.c               |  1 +
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  4 ++++
 security/security.c           | 11 +++++++++++
 4 files changed, 17 insertions(+)

Comments

Casey Schaufler Nov. 7, 2023, 5:39 p.m. UTC | #1
On 11/7/2023 5:40 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_pre_free_security hook.
>
> IMA calculates at file close the new digest of the file content and writes
> it to security.ima, so that appraisal at next file access succeeds.
>
> LSMs could also take some action before the last reference of a file is
> released.
>
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>


> ---
>  fs/file_table.c               |  1 +
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  4 ++++
>  security/security.c           | 11 +++++++++++
>  4 files changed, 17 insertions(+)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index de4a2915bfd4..64ed74555e64 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -385,6 +385,7 @@ static void __fput(struct file *file)
>  	eventpoll_release(file);
>  	locks_remove_file(file);
>  
> +	security_file_pre_free(file);
>  	ima_file_free(file);
>  	if (unlikely(file->f_flags & FASYNC)) {
>  		if (file->f_op->fasync)
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 4f6861fecacd..5d0a09ead7ac 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -173,6 +173,7 @@ LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
>  	 struct kernfs_node *kn)
>  LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
>  LSM_HOOK(int, 0, file_alloc_security, struct file *file)
> +LSM_HOOK(void, LSM_RET_VOID, file_pre_free_security, struct file *file)
>  LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>  LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>  	 unsigned long arg)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c360458920b1..a570213693d9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -395,6 +395,7 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir,
>  				  struct kernfs_node *kn);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
> +void security_file_pre_free(struct file *file);
>  void security_file_free(struct file *file);
>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>  int security_mmap_file(struct file *file, unsigned long prot,
> @@ -1006,6 +1007,9 @@ static inline int security_file_alloc(struct file *file)
>  	return 0;
>  }
>  
> +static inline void security_file_pre_free(struct file *file)
> +{ }
> +
>  static inline void security_file_free(struct file *file)
>  { }
>  
> diff --git a/security/security.c b/security/security.c
> index fe6a160afc35..331a3e5efb62 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2724,6 +2724,17 @@ int security_file_alloc(struct file *file)
>  	return rc;
>  }
>  
> +/**
> + * security_file_pre_free() - Perform actions before releasing the file ref
> + * @file: the file
> + *
> + * Perform actions before releasing the last reference to a file.
> + */
> +void security_file_pre_free(struct file *file)
> +{
> +	call_void_hook(file_pre_free_security, file);
> +}
> +
>  /**
>   * security_file_free() - Free a file's LSM blob
>   * @file: the file
Paul Moore Nov. 16, 2023, 4:33 a.m. UTC | #2
On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_pre_free_security hook.
> 
> IMA calculates at file close the new digest of the file content and writes
> it to security.ima, so that appraisal at next file access succeeds.
> 
> LSMs could also take some action before the last reference of a file is
> released.
> 
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  fs/file_table.c               |  1 +
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  4 ++++
>  security/security.c           | 11 +++++++++++
>  4 files changed, 17 insertions(+)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index de4a2915bfd4..64ed74555e64 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -385,6 +385,7 @@ static void __fput(struct file *file)
>  	eventpoll_release(file);
>  	locks_remove_file(file);
>  
> +	security_file_pre_free(file);

I worry that security_file_pre_free() is a misleading name as "free"
tends to imply memory management tasks, which isn't the main focus of
this hook.  What do you think of security_file_release() or
security_file_put() instead?

>  	ima_file_free(file);
>  	if (unlikely(file->f_flags & FASYNC)) {
>  		if (file->f_op->fasync)

--
paul-moore.com
Roberto Sassu Nov. 16, 2023, 9:46 a.m. UTC | #3
On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> > 
> > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > the file_pre_free_security hook.
> > 
> > IMA calculates at file close the new digest of the file content and writes
> > it to security.ima, so that appraisal at next file access succeeds.
> > 
> > LSMs could also take some action before the last reference of a file is
> > released.
> > 
> > The new hook cannot return an error and cannot cause the operation to be
> > reverted.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  fs/file_table.c               |  1 +
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  4 ++++
> >  security/security.c           | 11 +++++++++++
> >  4 files changed, 17 insertions(+)
> > 
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index de4a2915bfd4..64ed74555e64 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -385,6 +385,7 @@ static void __fput(struct file *file)
> >  	eventpoll_release(file);
> >  	locks_remove_file(file);
> >  
> > +	security_file_pre_free(file);
> 
> I worry that security_file_pre_free() is a misleading name as "free"
> tends to imply memory management tasks, which isn't the main focus of
> this hook.  What do you think of security_file_release() or
> security_file_put() instead?

security_file_release() would be fine for me.

Thanks

Roberto

> >  	ima_file_free(file);
> >  	if (unlikely(file->f_flags & FASYNC)) {
> >  		if (file->f_op->fasync)
> 
> --
> paul-moore.com
Paul Moore Nov. 16, 2023, 6:41 p.m. UTC | #4
On Thu, Nov 16, 2023 at 4:47 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote:
> > On Nov  7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> > >
> > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > > the file_pre_free_security hook.
> > >
> > > IMA calculates at file close the new digest of the file content and writes
> > > it to security.ima, so that appraisal at next file access succeeds.
> > >
> > > LSMs could also take some action before the last reference of a file is
> > > released.
> > >
> > > The new hook cannot return an error and cannot cause the operation to be
> > > reverted.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > ---
> > >  fs/file_table.c               |  1 +
> > >  include/linux/lsm_hook_defs.h |  1 +
> > >  include/linux/security.h      |  4 ++++
> > >  security/security.c           | 11 +++++++++++
> > >  4 files changed, 17 insertions(+)
> > >
> > > diff --git a/fs/file_table.c b/fs/file_table.c
> > > index de4a2915bfd4..64ed74555e64 100644
> > > --- a/fs/file_table.c
> > > +++ b/fs/file_table.c
> > > @@ -385,6 +385,7 @@ static void __fput(struct file *file)
> > >     eventpoll_release(file);
> > >     locks_remove_file(file);
> > >
> > > +   security_file_pre_free(file);
> >
> > I worry that security_file_pre_free() is a misleading name as "free"
> > tends to imply memory management tasks, which isn't the main focus of
> > this hook.  What do you think of security_file_release() or
> > security_file_put() instead?
>
> security_file_release() would be fine for me.

Okay, assuming no objections for anyone else let's go with that.
Thanks for indulging my naming nitpick :)
diff mbox series

Patch

diff --git a/fs/file_table.c b/fs/file_table.c
index de4a2915bfd4..64ed74555e64 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -385,6 +385,7 @@  static void __fput(struct file *file)
 	eventpoll_release(file);
 	locks_remove_file(file);
 
+	security_file_pre_free(file);
 	ima_file_free(file);
 	if (unlikely(file->f_flags & FASYNC)) {
 		if (file->f_op->fasync)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 4f6861fecacd..5d0a09ead7ac 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -173,6 +173,7 @@  LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
 	 struct kernfs_node *kn)
 LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
 LSM_HOOK(int, 0, file_alloc_security, struct file *file)
+LSM_HOOK(void, LSM_RET_VOID, file_pre_free_security, struct file *file)
 LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
 LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
 	 unsigned long arg)
diff --git a/include/linux/security.h b/include/linux/security.h
index c360458920b1..a570213693d9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -395,6 +395,7 @@  int security_kernfs_init_security(struct kernfs_node *kn_dir,
 				  struct kernfs_node *kn);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
+void security_file_pre_free(struct file *file);
 void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int security_mmap_file(struct file *file, unsigned long prot,
@@ -1006,6 +1007,9 @@  static inline int security_file_alloc(struct file *file)
 	return 0;
 }
 
+static inline void security_file_pre_free(struct file *file)
+{ }
+
 static inline void security_file_free(struct file *file)
 { }
 
diff --git a/security/security.c b/security/security.c
index fe6a160afc35..331a3e5efb62 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2724,6 +2724,17 @@  int security_file_alloc(struct file *file)
 	return rc;
 }
 
+/**
+ * security_file_pre_free() - Perform actions before releasing the file ref
+ * @file: the file
+ *
+ * Perform actions before releasing the last reference to a file.
+ */
+void security_file_pre_free(struct file *file)
+{
+	call_void_hook(file_pre_free_security, file);
+}
+
 /**
  * security_file_free() - Free a file's LSM blob
  * @file: the file