| 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 |
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
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
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
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 --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