| Message ID | 20221013223654.659758-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | integrity: Move hooks into LSM | expand |
On Thu, Oct 13, 2022 at 03:36:49PM -0700, Kees Cook wrote: > The file_free_security hook already exists for managing notification of > released files. Use the LSM hook instead of open-coded stacking. > > Cc: Mimi Zohar <zohar@linux.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: Petr Vorel <pvorel@suse.cz> > Cc: Jonathan McDowell <noodles@fb.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/file_table.c | 1 - > include/linux/ima.h | 6 ------ > security/integrity/ima/ima_main.c | 3 ++- > 3 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/fs/file_table.c b/fs/file_table.c > index 99c6796c9f28..fa707d221a43 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -311,7 +311,6 @@ static void __fput(struct file *file) > eventpoll_release(file); > locks_remove_file(file); > > - ima_file_free(file); > if (unlikely(file->f_flags & FASYNC)) { > if (file->f_op->fasync) > file->f_op->fasync(-1, file, 0); > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 6dc5143f89f2..9f18df366064 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -19,7 +19,6 @@ extern enum hash_algo ima_get_current_hash_algo(void); > extern int ima_file_check(struct file *file, int mask); > extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns, > struct inode *inode); > -extern void ima_file_free(struct file *file); > extern void ima_post_path_mknod(struct user_namespace *mnt_userns, > struct dentry *dentry); > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); > @@ -56,11 +55,6 @@ static inline void ima_post_create_tmpfile(struct user_namespace *mnt_userns, > { > } > > -static inline void ima_file_free(struct file *file) > -{ > - return; > -} > - > static inline void ima_post_path_mknod(struct user_namespace *mnt_userns, > struct dentry *dentry) > { > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index b3b79d030a67..94379ba40b58 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -183,7 +183,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, > * > * Flag files that changed, based on i_version > */ > -void ima_file_free(struct file *file) > +static void ima_file_free(struct file *file) > { > struct inode *inode = file_inode(file); > struct integrity_iint_cache *iint; > @@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > LSM_HOOK_INIT(mmap_file, ima_file_mmap), > LSM_HOOK_INIT(file_mprotect, ima_file_mprotect), > + LSM_HOOK_INIT(file_free_security, ima_file_free), This doesn't work afaict. If the file is opened for writing ima may update xattrs. But by the time security_file_free() is called put_file_access() has already been called which will have given up write access to the file's mount. So you would have to - just one of the possibilities - have to move security_file_free() out of file_free() and into the old ima_file_free() location. But that might cause semantic changes for other LSMs.
On Tue, 2022-10-18 at 17:02 +0200, Christian Brauner wrote: > On Thu, Oct 13, 2022 at 03:36:49PM -0700, Kees Cook wrote: > > The file_free_security hook already exists for managing > > notification of > > released files. Use the LSM hook instead of open-coded stacking. > > > > Cc: Mimi Zohar <zohar@linux.ibm.com> > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > > Cc: Paul Moore <paul@paul-moore.com> > > Cc: James Morris <jmorris@namei.org> > > Cc: "Serge E. Hallyn" <serge@hallyn.com> > > Cc: Petr Vorel <pvorel@suse.cz> > > Cc: Jonathan McDowell <noodles@fb.com> > > Cc: Borislav Petkov <bp@suse.de> > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: linux-integrity@vger.kernel.org > > Cc: linux-security-module@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > fs/file_table.c | 1 - > > include/linux/ima.h | 6 ------ > > security/integrity/ima/ima_main.c | 3 ++- > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/fs/file_table.c b/fs/file_table.c > > index 99c6796c9f28..fa707d221a43 100644 > > --- a/fs/file_table.c > > +++ b/fs/file_table.c > > @@ -311,7 +311,6 @@ static void __fput(struct file *file) > > eventpoll_release(file); > > locks_remove_file(file); > > > > - ima_file_free(file); > > if (unlikely(file->f_flags & FASYNC)) { > > if (file->f_op->fasync) > > file->f_op->fasync(-1, file, 0); > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > index 6dc5143f89f2..9f18df366064 100644 > > --- a/include/linux/ima.h > > +++ b/include/linux/ima.h > > @@ -19,7 +19,6 @@ extern enum hash_algo > > ima_get_current_hash_algo(void); > > extern int ima_file_check(struct file *file, int mask); > > extern void ima_post_create_tmpfile(struct user_namespace > > *mnt_userns, > > struct inode *inode); > > -extern void ima_file_free(struct file *file); > > extern void ima_post_path_mknod(struct user_namespace *mnt_userns, > > struct dentry *dentry); > > extern int ima_file_hash(struct file *file, char *buf, size_t > > buf_size); > > @@ -56,11 +55,6 @@ static inline void > > ima_post_create_tmpfile(struct user_namespace *mnt_userns, > > { > > } > > > > -static inline void ima_file_free(struct file *file) > > -{ > > - return; > > -} > > - > > static inline void ima_post_path_mknod(struct user_namespace > > *mnt_userns, > > struct dentry *dentry) > > { > > diff --git a/security/integrity/ima/ima_main.c > > b/security/integrity/ima/ima_main.c > > index b3b79d030a67..94379ba40b58 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -183,7 +183,7 @@ static void ima_check_last_writer(struct > > integrity_iint_cache *iint, > > * > > * Flag files that changed, based on i_version > > */ > > -void ima_file_free(struct file *file) > > +static void ima_file_free(struct file *file) > > { > > struct inode *inode = file_inode(file); > > struct integrity_iint_cache *iint; > > @@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[] > > __lsm_ro_after_init = { > > LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), > > LSM_HOOK_INIT(mmap_file, ima_file_mmap), > > LSM_HOOK_INIT(file_mprotect, ima_file_mprotect), > > + LSM_HOOK_INIT(file_free_security, ima_file_free), > > This doesn't work afaict. If the file is opened for writing ima may > update xattrs. But by the time security_file_free() is called > put_file_access() has already been called which will have given up > write > access to the file's mount. > > So you would have to - just one of the possibilities - have to move > security_file_free() out of file_free() and into the old > ima_file_free() > location. But that might cause semantic changes for other LSMs. Hi I also did this work before. In my implementation, I created a new security hook called security_file_pre_free(). https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263 If useful, the whole patch set is available at: https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3 Roberto
On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote: > I also did this work before. In my implementation, I created a new > security hook called security_file_pre_free(). > > https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263 > > If useful, the whole patch set is available at: > > https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3 Ah, lovely! Can you pick this back up and run with it? I mainly did these a proof-of-concept, but it looks like you got further.
On Tue, 2022-10-18 at 11:29 -0700, Kees Cook wrote: > On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote: > > I also did this work before. In my implementation, I created a new > > security hook called security_file_pre_free(). > > > > https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263 > > > > If useful, the whole patch set is available at: > > > > https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3 > > Ah, lovely! Can you pick this back up and run with it? I mainly did > these a proof-of-concept, but it looks like you got further. It was some time ago. If I remember correctly, I got to the point of running IMA/EVM and passing basic tests. Will take a look at your patches and comments, and integrate in mines if something is missing. Will also send again the prerequisite patch set. Roberto
On Wed, Oct 19, 2022 at 2:56 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Tue, 2022-10-18 at 11:29 -0700, Kees Cook wrote: > > On Tue, Oct 18, 2022 at 05:32:40PM +0200, Roberto Sassu wrote: > > > I also did this work before. In my implementation, I created a new > > > security hook called security_file_pre_free(). > > > > > > https://github.com/robertosassu/linux/commit/692c9d36fff865435b23b3cb765d31f3584f6263 > > > > > > If useful, the whole patch set is available at: > > > > > > https://github.com/robertosassu/linux/commits/ima-evm-lsm-v1-devel-v3 > > > > Ah, lovely! Can you pick this back up and run with it? I mainly did > > these a proof-of-concept, but it looks like you got further. > > It was some time ago. If I remember correctly, I got to the point of > running IMA/EVM and passing basic tests. > > Will take a look at your patches and comments, and integrate in mines > if something is missing. > > Will also send again the prerequisite patch set. Thanks Roberto, I appreciate you taking the time to resume your earlier work. I think this will be a nice improvement and help us cleanup a lot of code.
diff --git a/fs/file_table.c b/fs/file_table.c index 99c6796c9f28..fa707d221a43 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -311,7 +311,6 @@ static void __fput(struct file *file) eventpoll_release(file); locks_remove_file(file); - ima_file_free(file); if (unlikely(file->f_flags & FASYNC)) { if (file->f_op->fasync) file->f_op->fasync(-1, file, 0); diff --git a/include/linux/ima.h b/include/linux/ima.h index 6dc5143f89f2..9f18df366064 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -19,7 +19,6 @@ extern enum hash_algo ima_get_current_hash_algo(void); extern int ima_file_check(struct file *file, int mask); extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns, struct inode *inode); -extern void ima_file_free(struct file *file); extern void ima_post_path_mknod(struct user_namespace *mnt_userns, struct dentry *dentry); extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); @@ -56,11 +55,6 @@ static inline void ima_post_create_tmpfile(struct user_namespace *mnt_userns, { } -static inline void ima_file_free(struct file *file) -{ - return; -} - static inline void ima_post_path_mknod(struct user_namespace *mnt_userns, struct dentry *dentry) { diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index b3b79d030a67..94379ba40b58 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -183,7 +183,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, * * Flag files that changed, based on i_version */ -void ima_file_free(struct file *file) +static void ima_file_free(struct file *file) { struct inode *inode = file_inode(file); struct integrity_iint_cache *iint; @@ -1085,6 +1085,7 @@ static struct security_hook_list ima_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(bprm_check_security, ima_bprm_check), LSM_HOOK_INIT(mmap_file, ima_file_mmap), LSM_HOOK_INIT(file_mprotect, ima_file_mprotect), + LSM_HOOK_INIT(file_free_security, ima_file_free), LSM_HOOK_INIT(kernel_read_file, ima_read_file), LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file), LSM_HOOK_INIT(kernel_load_data, ima_load_data),
The file_free_security hook already exists for managing notification of released files. Use the LSM hook instead of open-coded stacking. Cc: Mimi Zohar <zohar@linux.ibm.com> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Petr Vorel <pvorel@suse.cz> Cc: Jonathan McDowell <noodles@fb.com> Cc: Borislav Petkov <bp@suse.de> Cc: Takashi Iwai <tiwai@suse.de> Cc: linux-integrity@vger.kernel.org Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/file_table.c | 1 - include/linux/ima.h | 6 ------ security/integrity/ima/ima_main.c | 3 ++- 3 files changed, 2 insertions(+), 8 deletions(-)