| Message ID | 20190410145659.26347-1-janne.karhunen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: make 'sync' update the inode integrity state | expand |
Hi Janne, I need to finish up a couple of other things before vacation. Below are just a few comments/questions for you to think about. On Wed, 2019-04-10 at 17:56 +0300, Janne Karhunen wrote: > +/** > + * ima_file_update - called from sync to update xattrs > + * @file: pointer to file structure being updated > + */ > +void ima_file_update(struct file *file) > +{ > + struct inode *inode = file_inode(file); > + struct integrity_iint_cache *iint; > + > + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) > + return; > + > + iint = integrity_iint_find(inode); > + if (!iint) > + return; > + > + mutex_lock(&iint->mutex); > + if (atomic_read(&inode->i_writecount) == 1) { This test limits the number of opened writers. Only if there is a single writer opened, will the xattr be updated. Is this what you intended? Your testing should open the same file for write multiple times. > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); > + if (!IS_I_VERSION(inode) || > + !inode_eq_iversion(inode, iint->version)) { > + iint->flags &= ~IMA_COLLECTED; > + ima_update_xattr(iint, file); Relatively recently there were some changes to iversion so that it isn't being updated as frequently. Can we use i_version here? > + } > + } > + mutex_unlock(&iint->mutex); > +} > +EXPORT_SYMBOL_GPL(ima_file_update); > + > /** > * ima_path_check - based on policy, collect/store measurement. > * @file: pointer to the file to be measured > diff --git a/security/security.c b/security/security.c > index 23cbb1a295a3..6a0980a1df22 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1451,6 +1451,11 @@ int security_file_open(struct file *file) > return fsnotify_perm(file, MAY_OPEN); > } > > +void security_file_sync(struct file *file) > +{ > + ima_file_update(file); > +} > + Either this is an LSM hook or it isn't. If it's an LSM hook it needs to be similar to the existing hooks. If it's an IMA hook, like ima_file_check() or ima_file_free(), then call it directly. Normally the function name is related to the LSM hook name. For example, I would name it ima_file_sync. Mimi > int security_task_alloc(struct task_struct *task, unsigned long clone_flags) > { > int rc = lsm_task_alloc(task);
On Thu, Apr 11, 2019 at 4:03 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > + mutex_lock(&iint->mutex); > > + if (atomic_read(&inode->i_writecount) == 1) { > > This test limits the number of opened writers. Only if there is a > single writer opened, will the xattr be updated. Is this what you > intended? Certainly no, I misunderstood that to be a flag. Will fix. > Your testing should open the same file for write multiple times. Indeed. I was just wondering what happened with 'locksettings.db' as it failed to update. Duh.. > > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); > > + if (!IS_I_VERSION(inode) || > > + !inode_eq_iversion(inode, iint->version)) { > > + iint->flags &= ~IMA_COLLECTED; > > + ima_update_xattr(iint, file); > > Relatively recently there were some changes to iversion so that it > isn't being updated as frequently. Can we use i_version here? Thanks. > > +void security_file_sync(struct file *file) > > +{ > > + ima_file_update(file); > > +} > > + > > Either this is an LSM hook or it isn't. If it's an LSM hook it needs > to be similar to the existing hooks. If it's an IMA hook, like > ima_file_check() or ima_file_free(), then call it directly. > > Normally the function name is related to the LSM hook name. For > example, I would name it ima_file_sync. Ok. I guess ima hook it is then, doubt any lsm will ever need it? -- Janne
Hi Mimi, I will take a couple of days to test and think this through a bit better, there are extra cases like file creation without ever writing to it (also ending up in appraisal failure for no reason) and msync() that would also be beneficial to tackle on the same go. -- Janne On Thu, Apr 11, 2019 at 4:03 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Hi Janne, > > I need to finish up a couple of other things before vacation. Below > are just a few comments/questions for you to think about. > > On Wed, 2019-04-10 at 17:56 +0300, Janne Karhunen wrote: > > > +/** > > + * ima_file_update - called from sync to update xattrs > > + * @file: pointer to file structure being updated > > + */ > > +void ima_file_update(struct file *file) > > +{ > > + struct inode *inode = file_inode(file); > > + struct integrity_iint_cache *iint; > > + > > + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) > > + return; > > + > > + iint = integrity_iint_find(inode); > > + if (!iint) > > + return; > > + > > + mutex_lock(&iint->mutex); > > + if (atomic_read(&inode->i_writecount) == 1) { > > This test limits the number of opened writers. Only if there is a > single writer opened, will the xattr be updated. Is this what you > intended? > > Your testing should open the same file for write multiple times. > > > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); > > + if (!IS_I_VERSION(inode) || > > + !inode_eq_iversion(inode, iint->version)) { > > + iint->flags &= ~IMA_COLLECTED; > > + ima_update_xattr(iint, file); > > Relatively recently there were some changes to iversion so that it > isn't being updated as frequently. Can we use i_version here? > > > + } > > + } > > + mutex_unlock(&iint->mutex); > > +} > > +EXPORT_SYMBOL_GPL(ima_file_update); > > + > > /** > > * ima_path_check - based on policy, collect/store measurement. > > * @file: pointer to the file to be measured > > diff --git a/security/security.c b/security/security.c > > index 23cbb1a295a3..6a0980a1df22 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1451,6 +1451,11 @@ int security_file_open(struct file *file) > > return fsnotify_perm(file, MAY_OPEN); > > } > > > > +void security_file_sync(struct file *file) > > +{ > > + ima_file_update(file); > > +} > > + > > Either this is an LSM hook or it isn't. If it's an LSM hook it needs > to be similar to the existing hooks. If it's an IMA hook, like > ima_file_check() or ima_file_free(), then call it directly. > > Normally the function name is related to the LSM hook name. For > example, I would name it ima_file_sync. > > Mimi > > > int security_task_alloc(struct task_struct *task, unsigned long clone_flags) > > { > > int rc = lsm_task_alloc(task); >
Hi, Finally. Now I think I'm almost happy with the overall construction and my gut feeling is telling me I can make the hashes reflect the filesystem state pretty well, maybe even over 99.9% of the time given the workload stock android is generating. The pieces that we did: - initialize hashes correctly on open() - hook 'sync' - hook 'msync' - hook 'truncate' - introduce a per-cpu workqueue that gets work items from write and dirty page flush. Long-running write is allowed to go as is (no performance penalty from re-measurements), but once the the write goes silent workqueue item is flushed and the file is hashed. Before I uplift this construction against the mainline, any thoughts about this? All I can say so far is that it runs and it seems to keep the system relatively crash tolerant. 'Don't let the perfect be the enemy of the better' I guess... -- Janne On Fri, Apr 12, 2019 at 3:40 PM Janne Karhunen <janne.karhunen@gmail.com> wrote: > > Hi Mimi, > > I will take a couple of days to test and think this through a bit > better, there are extra cases like file creation without ever writing > to it (also ending up in appraisal failure for no reason) and msync() > that would also be beneficial to tackle on the same go. > > > -- > Janne > > > On Thu, Apr 11, 2019 at 4:03 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > Hi Janne, > > > > I need to finish up a couple of other things before vacation. Below > > are just a few comments/questions for you to think about. > > > > On Wed, 2019-04-10 at 17:56 +0300, Janne Karhunen wrote: > > > > > +/** > > > + * ima_file_update - called from sync to update xattrs > > > + * @file: pointer to file structure being updated > > > + */ > > > +void ima_file_update(struct file *file) > > > +{ > > > + struct inode *inode = file_inode(file); > > > + struct integrity_iint_cache *iint; > > > + > > > + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) > > > + return; > > > + > > > + iint = integrity_iint_find(inode); > > > + if (!iint) > > > + return; > > > + > > > + mutex_lock(&iint->mutex); > > > + if (atomic_read(&inode->i_writecount) == 1) { > > > > This test limits the number of opened writers. Only if there is a > > single writer opened, will the xattr be updated. Is this what you > > intended? > > > > Your testing should open the same file for write multiple times. > > > > > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); > > > + if (!IS_I_VERSION(inode) || > > > + !inode_eq_iversion(inode, iint->version)) { > > > + iint->flags &= ~IMA_COLLECTED; > > > + ima_update_xattr(iint, file); > > > > Relatively recently there were some changes to iversion so that it > > isn't being updated as frequently. Can we use i_version here? > > > > > + } > > > + } > > > + mutex_unlock(&iint->mutex); > > > +} > > > +EXPORT_SYMBOL_GPL(ima_file_update); > > > + > > > /** > > > * ima_path_check - based on policy, collect/store measurement. > > > * @file: pointer to the file to be measured > > > diff --git a/security/security.c b/security/security.c > > > index 23cbb1a295a3..6a0980a1df22 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -1451,6 +1451,11 @@ int security_file_open(struct file *file) > > > return fsnotify_perm(file, MAY_OPEN); > > > } > > > > > > +void security_file_sync(struct file *file) > > > +{ > > > + ima_file_update(file); > > > +} > > > + > > > > Either this is an LSM hook or it isn't. If it's an LSM hook it needs > > to be similar to the existing hooks. If it's an IMA hook, like > > ima_file_check() or ima_file_free(), then call it directly. > > > > Normally the function name is related to the LSM hook name. For > > example, I would name it ima_file_sync. > > > > Mimi > > > > > int security_task_alloc(struct task_struct *task, unsigned long clone_flags) > > > { > > > int rc = lsm_task_alloc(task); > >
On Thu, 2019-04-25 at 13:05 +0300, Janne Karhunen wrote: > Hi, > Finally. Now I think I'm almost happy with the overall construction > and my gut feeling is telling me I can make the hashes reflect the > filesystem state pretty well, maybe even over 99.9% of the time given > the workload stock android is generating. > > The pieces that we did: > - initialize hashes correctly on open() > - hook 'sync' > - hook 'msync' > - hook 'truncate' > - introduce a per-cpu workqueue that gets work items from write and > dirty page flush. Long-running write is allowed to go as is (no > performance penalty from re-measurements), but once the the write goes > silent workqueue item is flushed and the file is hashed. What type of workqueue (eg. fifo)? Are all of the writes needed, as they might be re-written later. > > Before I uplift this construction against the mainline, any thoughts > about this? All I can say so far is that it runs and it seems to keep > the system relatively crash tolerant. 'Don't let the perfect be the > enemy of the better' I guess... As long as the iint cache info isn't affected by these changes, the change is limited to writing out the xattr more frequently. If the iint cache info is affected, then you need to be careful that multiple readers/writers continue to work. Mimi
On Thu, Apr 25, 2019 at 3:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > The pieces that we did: > > - initialize hashes correctly on open() > > - hook 'sync' > > - hook 'msync' > > - hook 'truncate' > > - introduce a per-cpu workqueue that gets work items from write and > > dirty page flush. Long-running write is allowed to go as is (no > > performance penalty from re-measurements), but once the the write goes > > silent workqueue item is flushed and the file is hashed. > > What type of workqueue (eg. fifo)? Are all of the writes needed, as > they might be re-written later. We can talk about this, but what I have now is static struct workqueue_struct *ima_update_wq; ima_update_wq = create_workqueue("ima-update-wq"); .. queue_work(ima_update_wq, &iint->ima_update_data.work); All writes are not hashed at all, it tries to insert an element into the workqueue on every write, but if there is already one it is happy. Alternative is that we add delayed work that is constantly kicked forward, so rehash will fire for example 'dirty_expire_centisecs' after the last write on the inode. Unfortunately I need really high crash tolerance in my use case (lovely vendor kernel), so I don't delay it much in our case. Android is pretty rough test, btw. It keeps 800+ files open for writing on /data alone. This survives that kind of thing crashing in a loop. > > Before I uplift this construction against the mainline, any thoughts > > about this? All I can say so far is that it runs and it seems to keep > > the system relatively crash tolerant. 'Don't let the perfect be the > > enemy of the better' I guess... > > As long as the iint cache info isn't affected by these changes, the > change is limited to writing out the xattr more frequently. If the > iint cache info is affected, then you need to be careful that multiple > readers/writers continue to work. Sadly, it is affected. That is where I hide the 'work_struct' and the 'file' pointer :-/ -- Janne
On Thu, 2019-04-11 at 09:03 -0400, Mimi Zohar wrote: > On Wed, 2019-04-10 at 17:56 +0300, Janne Karhunen wrote: > > > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); > > + if (!IS_I_VERSION(inode) || > > + !inode_eq_iversion(inode, iint->version)) { > > + iint->flags &= ~IMA_COLLECTED; > > + ima_update_xattr(iint, file); > > Relatively recently there were some changes to iversion so that it > isn't being updated as frequently. Can we use i_version here? I was referring to Jeff Layton's i_version changes.[1] Mimi [1] f02a9ad1f15d ("fs: handle inode->i_version more efficiently") > > > + } > > + } > > + mutex_unlock(&iint->mutex); > > +} > > +EXPORT_SYMBOL_GPL(ima_file_update); > > + > > /** > > * ima_path_check - based on policy, collect/store measurement. > > * @file: pointer to the file to be measured
On Fri, May 3, 2019 at 2:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > [1] f02a9ad1f15d ("fs: handle inode->i_version more efficiently") > > > > Thank you. I wasn't aware of this and it makes sense, > > The question is whether this impacts your current set of patches? We > really need to take this discussion back to the mailing list. I've > responded to my original post with this info. Hmm, if we add the iversion query as you suggested the function can be added as a polling point just about anywhere. I added it in there now. I sent the current state of the upstream port as a new patch with a proper topic. The mm page scanning code is so rough that it is still omitted, let's continue the discussion from the core plumbing first. I did not adopt to your function naming changes yet as the scope of the work is now much wider, plain 'ima_file_sync' may be misleading as we also follow the data as it's being written. Before you ask, the reason why it now does the periodic measurements of the files as they are written instead of doing 'mod_delayed_work' timer kicking is that the 'mod_delayed_work' seems quite heavy. Now there is almost no added latency to the 'write()' loop, just random measurements with freely definable intervals. -- Janne
> > The question is whether this impacts your current set of patches? We > > really need to take this discussion back to the mailing list. I've > > responded to my original post with this info. > > Hmm, if we add the iversion query as you suggested the function can be > added as a polling point just about anywhere. I added it in there now. That said, before I chop that patch into pieces with individual explanations (and fix the few remaining bugs that seemed to have slipped in), hope you can let me know beforehand if the concept is ok from your point of view. I understand it fully if this is too intrusive as it goes all over the place. Just something we had to do to keep the system rolling. -- Janne
diff --git a/fs/sync.c b/fs/sync.c index b54e0541ad89..6c01f1970eac 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -16,6 +16,7 @@ #include <linux/pagemap.h> #include <linux/quotaops.h> #include <linux/backing-dev.h> +#include <linux/security.h> #include "internal.h" #define VALID_FLAGS (SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE| \ @@ -194,6 +195,8 @@ int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int datasync) return -EINVAL; if (!datasync && (inode->i_state & I_DIRTY_TIME)) mark_inode_dirty_sync(inode); + security_file_sync(file); + return file->f_op->fsync(file, start, end, datasync); } EXPORT_SYMBOL(vfs_fsync_range); diff --git a/include/linux/ima.h b/include/linux/ima.h index dc12fbcf484c..dbd6c56ae7e1 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -20,6 +20,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm); extern int ima_file_check(struct file *file, int mask); extern void ima_post_create_tmpfile(struct inode *inode); extern void ima_file_free(struct file *file); +extern void ima_file_update(struct file *file); extern int ima_file_mmap(struct file *file, unsigned long prot); extern int ima_load_data(enum kernel_load_data_id id); extern int ima_read_file(struct file *file, enum kernel_read_file_id id); diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index a9b8ff578b6b..196a04d06fb6 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -541,6 +541,9 @@ * Save open-time permission checking state for later use upon * file_permission, and recheck access if anything has changed * since inode_permission. + * @file_sync: + * Called when sync is being requested for file. Primary use is to make + * sure integrity measurement of the file is kept in sync with the data. * * Security hooks for task operations. * @@ -1596,6 +1599,7 @@ union security_list_options { struct fown_struct *fown, int sig); int (*file_receive)(struct file *file); int (*file_open)(struct file *file); + void (*file_sync)(struct file *file); int (*task_alloc)(struct task_struct *task, unsigned long clone_flags); void (*task_free)(struct task_struct *task); @@ -1892,6 +1896,7 @@ struct security_hook_heads { struct hlist_head file_send_sigiotask; struct hlist_head file_receive; struct hlist_head file_open; + struct hlist_head file_sync; struct hlist_head task_alloc; struct hlist_head task_free; struct hlist_head cred_alloc_blank; diff --git a/include/linux/security.h b/include/linux/security.h index 49f2685324b0..467cfa02f9f6 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -315,6 +315,7 @@ int security_file_send_sigiotask(struct task_struct *tsk, struct fown_struct *fown, int sig); int security_file_receive(struct file *file); int security_file_open(struct file *file); +void security_file_sync(struct file *file); int security_task_alloc(struct task_struct *task, unsigned long clone_flags); void security_task_free(struct task_struct *task); int security_cred_alloc_blank(struct cred *cred, gfp_t gfp); @@ -876,6 +877,11 @@ static inline int security_file_open(struct file *file) return 0; } +static inline void security_file_sync(struct file *file) +{ + return; +} + static inline int security_task_alloc(struct task_struct *task, unsigned long clone_flags) { diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 357edd140c09..e60b878d8356 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -16,7 +16,7 @@ * * File: ima_main.c * implements the IMA hooks: ima_bprm_check, ima_file_mmap, - * and ima_file_check. + * ima_file_update and ima_file_check. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -375,6 +375,35 @@ int ima_bprm_check(struct linux_binprm *bprm) MAY_EXEC, CREDS_CHECK); } +/** + * ima_file_update - called from sync to update xattrs + * @file: pointer to file structure being updated + */ +void ima_file_update(struct file *file) +{ + struct inode *inode = file_inode(file); + struct integrity_iint_cache *iint; + + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) + return; + + iint = integrity_iint_find(inode); + if (!iint) + return; + + mutex_lock(&iint->mutex); + if (atomic_read(&inode->i_writecount) == 1) { + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); + if (!IS_I_VERSION(inode) || + !inode_eq_iversion(inode, iint->version)) { + iint->flags &= ~IMA_COLLECTED; + ima_update_xattr(iint, file); + } + } + mutex_unlock(&iint->mutex); +} +EXPORT_SYMBOL_GPL(ima_file_update); + /** * ima_path_check - based on policy, collect/store measurement. * @file: pointer to the file to be measured diff --git a/security/security.c b/security/security.c index 23cbb1a295a3..6a0980a1df22 100644 --- a/security/security.c +++ b/security/security.c @@ -1451,6 +1451,11 @@ int security_file_open(struct file *file) return fsnotify_perm(file, MAY_OPEN); } +void security_file_sync(struct file *file) +{ + ima_file_update(file); +} + int security_task_alloc(struct task_struct *task, unsigned long clone_flags) { int rc = lsm_task_alloc(task);