diff mbox series

[v9,13/25] security: Introduce file_release hook

Message ID 20240115181809.885385-14-roberto.sassu@huaweicloud.com
State New
Headers
Series security: Move IMA and EVM to the LSM infrastructure |

Commit Message

Roberto Sassu Jan. 15, 2024, 6:17 p.m. UTC 
  From: Roberto Sassu <roberto.sassu@huawei.com>

In preparation for moving IMA and EVM to the LSM infrastructure, introduce
the file_release 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.

An LSM could implement an exclusive access scheme for files, only allowing
access to files that have no references.

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

Al Viro Jan. 15, 2024, 7:15 p.m. UTC | #1 
On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_release 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.
> 
> An LSM could implement an exclusive access scheme for files, only allowing
> access to files that have no references.

Elaborate that last part, please.
Roberto Sassu Jan. 16, 2024, 8:47 a.m. UTC | #2 
On Mon, 2024-01-15 at 19:15 +0000, Al Viro wrote:
> On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> > the file_release 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.
> > 
> > An LSM could implement an exclusive access scheme for files, only allowing
> > access to files that have no references.
> 
> Elaborate that last part, please.

Apologies, I didn't understand that either. Casey?

Thanks

Roberto
Casey Schaufler Jan. 16, 2024, 4:51 p.m. UTC | #3 
On 1/16/2024 12:47 AM, Roberto Sassu wrote:
> On Mon, 2024-01-15 at 19:15 +0000, Al Viro wrote:
>> On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>
>>> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
>>> the file_release 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.
>>>
>>> An LSM could implement an exclusive access scheme for files, only allowing
>>> access to files that have no references.
>> Elaborate that last part, please.
> Apologies, I didn't understand that either. Casey?

Just a hypothetical notion that if an LSM wanted to implement an
exclusive access scheme it might find the proposed hook helpful.
I don't have any plan to create such a scheme, nor do I think that
a file_release hook would be the only thing you'd need.

>
> Thanks
>
> Roberto
>
Al Viro Jan. 16, 2024, 5:33 p.m. UTC | #4 
On Tue, Jan 16, 2024 at 08:51:11AM -0800, Casey Schaufler wrote:
> On 1/16/2024 12:47 AM, Roberto Sassu wrote:
> > On Mon, 2024-01-15 at 19:15 +0000, Al Viro wrote:
> >> On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
> >>> From: Roberto Sassu <roberto.sassu@huawei.com>
> >>>
> >>> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> >>> the file_release 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.
> >>>
> >>> An LSM could implement an exclusive access scheme for files, only allowing
> >>> access to files that have no references.
> >> Elaborate that last part, please.
> > Apologies, I didn't understand that either. Casey?
> 
> Just a hypothetical notion that if an LSM wanted to implement an
> exclusive access scheme it might find the proposed hook helpful.
> I don't have any plan to create such a scheme, nor do I think that
> a file_release hook would be the only thing you'd need.

Exclusive access to what?  "No more than one opened file with this
inode at a time"?  It won't serialize IO operations, obviously...
Details, please.
Casey Schaufler Jan. 16, 2024, 6:18 p.m. UTC | #5 
On 1/16/2024 9:33 AM, Al Viro wrote:
> On Tue, Jan 16, 2024 at 08:51:11AM -0800, Casey Schaufler wrote:
>> On 1/16/2024 12:47 AM, Roberto Sassu wrote:
>>> On Mon, 2024-01-15 at 19:15 +0000, Al Viro wrote:
>>>> On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
>>>>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>
>>>>> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
>>>>> the file_release 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.
>>>>>
>>>>> An LSM could implement an exclusive access scheme for files, only allowing
>>>>> access to files that have no references.
>>>> Elaborate that last part, please.
>>> Apologies, I didn't understand that either. Casey?
>> Just a hypothetical notion that if an LSM wanted to implement an
>> exclusive access scheme it might find the proposed hook helpful.
>> I don't have any plan to create such a scheme, nor do I think that
>> a file_release hook would be the only thing you'd need.
> Exclusive access to what?  "No more than one opened file with this
> inode at a time"?  It won't serialize IO operations, obviously...
> Details, please.

Once a file is opened it can't be opened again until it is closed.
That's the simple description, which ignores all sorts of cases.
I wouldn't want my system to behave that way, but I have heard
arguments that multiple concurrent opens can be a security issue.
In the context of my review of the code in question I included
the comment solely for the purpose of acknowledging the potential
for additional uses of the proposed hook. It's entirely possible
someone (not me!) would use the hook in this or some other "clever"
way.
Paul Moore Feb. 8, 2024, 3:18 a.m. UTC | #6 
On Jan 15, 2024 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_release 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.
> 
> An LSM could implement an exclusive access scheme for files, only allowing
> access to files that have no references.

Let's drop the above sentence as it is is a little vague and is causing
some concern with the VFS folks.  While I want to see the hooks explained
and documented in the code, I've never been a big fan of speculating
about potential future uses of the hook, that's dangerous IMO.

Otherwise this looks good.

Acked-by: Paul Moore <paul@paul-moore.com>

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

--
paul-moore.com
Christian Brauner Feb. 9, 2024, 10:15 a.m. UTC | #7 
On Mon, Jan 15, 2024 at 07:17:57PM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_release 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.
> 
> An LSM could implement an exclusive access scheme for files, only allowing
> access to files that have no references.
> 
> 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(+)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index de4a2915bfd4..c72dc75f2bd3 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_release(file);
>  	ima_file_free(file);

This has always been an extremely dicy hook in here and that's caused us
issues before for stacking filesystems so I'm not enthusiastic about
exposing this to all LSMs. So reluctantly,

Acked-by: Christian Brauner <brauner@kernel.org>
Stefan Berger Feb. 12, 2024, 5:21 p.m. UTC | #8 
On 1/15/24 13:17, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation for moving IMA and EVM to the LSM infrastructure, introduce
> the file_release 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.
> 
> An LSM could implement an exclusive access scheme for files, only allowing
> access to files that have no references.
> 
> The new hook cannot return an error and cannot cause the operation to be
> reverted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Stefan Berger <stefanb@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..c72dc75f2bd3 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_release(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 c3fecc7dcb0b..229f84ce12ae 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_release, 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 97f2212c13b6..2997348afcb7 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_release(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_file_ioctl_compat(struct file *file, unsigned int cmd,
> @@ -1008,6 +1009,9 @@ static inline int security_file_alloc(struct file *file)
>   	return 0;
>   }
>   
> +static inline void security_file_release(struct file *file)
> +{ }
> +
>   static inline void security_file_free(struct file *file)
>   { }
>   
> diff --git a/security/security.c b/security/security.c
> index f3d92bffd02f..7d10724872f8 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2724,6 +2724,17 @@ int security_file_alloc(struct file *file)
>   	return rc;
>   }
>   
> +/**
> + * security_file_release() - Perform actions before releasing the file ref
> + * @file: the file
> + *
> + * Perform actions before releasing the last reference to a file.
> + */
> +void security_file_release(struct file *file)
> +{
> +	call_void_hook(file_release, file);
> +}
> +
>   /**
>    * security_file_free() - Free a file's LSM blob
>    * @file: the file
diff mbox series

Patch

diff --git a/fs/file_table.c b/fs/file_table.c
index de4a2915bfd4..c72dc75f2bd3 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_release(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 c3fecc7dcb0b..229f84ce12ae 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_release, 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 97f2212c13b6..2997348afcb7 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_release(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_file_ioctl_compat(struct file *file, unsigned int cmd,
@@ -1008,6 +1009,9 @@  static inline int security_file_alloc(struct file *file)
 	return 0;
 }
 
+static inline void security_file_release(struct file *file)
+{ }
+
 static inline void security_file_free(struct file *file)
 { }
 
diff --git a/security/security.c b/security/security.c
index f3d92bffd02f..7d10724872f8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2724,6 +2724,17 @@  int security_file_alloc(struct file *file)
 	return rc;
 }
 
+/**
+ * security_file_release() - Perform actions before releasing the file ref
+ * @file: the file
+ *
+ * Perform actions before releasing the last reference to a file.
+ */
+void security_file_release(struct file *file)
+{
+	call_void_hook(file_release, file);
+}
+
 /**
  * security_file_free() - Free a file's LSM blob
  * @file: the file