* [RFC PATCH] ima: audit log files opened with O_DIRECT flag @ 2014-05-12 20:24 Mimi Zohar 2014-05-12 23:27 ` Mimi Zohar 0 siblings, 1 reply; 6+ messages in thread From: Mimi Zohar @ 2014-05-12 20:24 UTC (permalink / raw) To: linux-security-module Cc: J. R. Okajima, Dmitry Kasatkin, Al Viro, linux-kernel As a temporary fix, do not measure, appraise, or audit files opened with the O_DIRECT flag set. Just audit log it. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- security/integrity/ima/ima_api.c | 10 +++++++++- security/integrity/ima/ima_main.c | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index ba9e4d7..d719978 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -199,6 +199,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, struct evm_ima_xattr_data **xattr_value, int *xattr_len) { + const char *audit_cause = "failed"; struct inode *inode = file_inode(file); const char *filename = file->f_dentry->d_name.name; int result = 0; @@ -213,6 +214,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, if (!(iint->flags & IMA_COLLECTED)) { u64 i_version = file_inode(file)->i_version; + if (file->f_flags & O_DIRECT) { + audit_cause = "failed(directio)"; + result = -EACCES; + goto out; + } + /* use default hash algorithm */ hash.hdr.algo = ima_hash_algo; @@ -233,9 +240,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, result = -ENOMEM; } } +out: if (result) integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, - filename, "collect_data", "failed", + filename, "collect_data", audit_cause, result, 0); return result; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 654111f..3e5b732 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -214,8 +214,11 @@ static int process_measurement(struct file *file, const char *filename, xattr_ptr = &xattr_value; rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len); - if (rc != 0) + if (rc != 0) { + if (file->f_flags & O_DIRECT) + rc = 0; goto out_digsig; + } pathname = filename ?: ima_d_path(&file->f_path, &pathbuf); -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] ima: audit log files opened with O_DIRECT flag 2014-05-12 20:24 [RFC PATCH] ima: audit log files opened with O_DIRECT flag Mimi Zohar @ 2014-05-12 23:27 ` Mimi Zohar 2014-05-13 7:19 ` J. R. Okajima 0 siblings, 1 reply; 6+ messages in thread From: Mimi Zohar @ 2014-05-12 23:27 UTC (permalink / raw) To: linux-security-module Cc: J. R. Okajima, Dmitry Kasatkin, Al Viro, linux-kernel Reposting unmangled version ... As a temporary fix, do not measure, appraise, or audit files opened with the O_DIRECT flag set. Just audit log it. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> Cc: <stable@vger.kernel.org> --- security/integrity/ima/ima_api.c | 10 +++++++++- security/integrity/ima/ima_main.c | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index ba9e4d7..d719978 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -199,6 +199,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, struct evm_ima_xattr_data **xattr_value, int *xattr_len) { + const char *audit_cause = "failed"; struct inode *inode = file_inode(file); const char *filename = file->f_dentry->d_name.name; int result = 0; @@ -213,6 +214,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, if (!(iint->flags & IMA_COLLECTED)) { u64 i_version = file_inode(file)->i_version; + if (file->f_flags & O_DIRECT) { + audit_cause = "failed(directio)"; + result = -EACCES; + goto out; + } + /* use default hash algorithm */ hash.hdr.algo = ima_hash_algo; @@ -233,9 +240,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, result = -ENOMEM; } } +out: if (result) integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, - filename, "collect_data", "failed", + filename, "collect_data", audit_cause, result, 0); return result; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 654111f..3e5b732 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -214,8 +214,11 @@ static int process_measurement(struct file *file, const char *filename, xattr_ptr = &xattr_value; rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len); - if (rc != 0) + if (rc != 0) { + if (file->f_flags & O_DIRECT) + rc = 0; goto out_digsig; + } pathname = filename ?: ima_d_path(&file->f_path, &pathbuf); -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] ima: audit log files opened with O_DIRECT flag 2014-05-12 23:27 ` Mimi Zohar @ 2014-05-13 7:19 ` J. R. Okajima 2014-05-13 11:10 ` Mimi Zohar 2014-05-19 21:06 ` [PATCH v1] " Mimi Zohar 0 siblings, 2 replies; 6+ messages in thread From: J. R. Okajima @ 2014-05-13 7:19 UTC (permalink / raw) To: Mimi Zohar; +Cc: linux-security-module, Dmitry Kasatkin, Al Viro, linux-kernel Mimi Zohar: > As a temporary fix, do not measure, appraise, or audit files > opened with the O_DIRECT flag set. Just audit log it. I have no objection about the patch, but have a question. Are you intending to put it into mainline now (and stable too)? Or is this a local bandage for whoever have met the problem (like me)? In other words, should I wait for another lock free solution from Dmitry Kasatkin? By the way, the mail is not delivered to stable-ML while there is "Cc: stable..." line in the commit log. J. R. Okajima ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] ima: audit log files opened with O_DIRECT flag 2014-05-13 7:19 ` J. R. Okajima @ 2014-05-13 11:10 ` Mimi Zohar 2014-05-19 21:06 ` [PATCH v1] " Mimi Zohar 1 sibling, 0 replies; 6+ messages in thread From: Mimi Zohar @ 2014-05-13 11:10 UTC (permalink / raw) To: J. R. Okajima Cc: linux-security-module, Dmitry Kasatkin, Al Viro, linux-kernel On Tue, 2014-05-13 at 16:19 +0900, J. R. Okajima wrote: > Mimi Zohar: > > As a temporary fix, do not measure, appraise, or audit files > > opened with the O_DIRECT flag set. Just audit log it. > > I have no objection about the patch, but have a question. > Are you intending to put it into mainline now (and stable too)? Or is > this a local bandage for whoever have met the problem (like me)? Yes, for the time being. > In other words, should I wait for another lock free solution from Dmitry > Kasatkin? In addition to the lockdep issue, there is a separate problem of reading the file opened w/O_DIRECT flag. Dmitry is about to post two patches, with a detailed explanation. The first patch re-introduces the iint->mutex. The other patch reads the file opened w/O_DIRECT flag. Neither patch is trivial. > By the way, the mail is not delivered to stable-ML while there is > "Cc: stable..." line in the commit log. The CC shouldn't have been included in an RFC. Mimi ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1] ima: audit log files opened with O_DIRECT flag 2014-05-13 7:19 ` J. R. Okajima 2014-05-13 11:10 ` Mimi Zohar @ 2014-05-19 21:06 ` Mimi Zohar 2014-05-21 9:38 ` Dmitry Kasatkin 1 sibling, 1 reply; 6+ messages in thread From: Mimi Zohar @ 2014-05-19 21:06 UTC (permalink / raw) To: J. R. Okajima Cc: linux-security-module, Dmitry Kasatkin, Al Viro, linux-kernel Files are measured or appraised based on the IMA policy. When a file, in policy, is opened with the O_DIRECT flag, a deadlock occurs. The first attempt at resolving this lockdep temporarily removed the O_DIRECT flag and restored it, after calculating the hash. The second attempt introduced the O_DIRECT_HAVELOCK flag. Based on this flag, do_blockdev_direct_IO() would skip taking the i_mutex a second time. The third attempt, by Dmitry Kasatkin, resolves the i_mutex locking issue, by re-introducing the IMA mutex, but uncovered another problem. Reading a file with O_DIRECT flag set, writes directly to userspace pages. A second patch allocates a user-space like memory. This works for all IMA hooks, except ima_file_free(), which is called on __fput() to recalculate the file hash. Until this last issue is addressed, do not 'collect' the measurement for measuring, appraising, or auditing files opened with the O_DIRECT flag set. Based on policy, permit or deny file access. This patch defines a new IMA policy rule option named 'permit_directio'. Policy rules could be defined, based on LSM or other criteria, to permit specific applications to open files with the O_DIRECT flag set. Changelog v1: - permit or deny file access based IMA policy rules Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- Documentation/ABI/testing/ima_policy | 2 +- security/integrity/ima/ima_api.c | 10 +++++++++- security/integrity/ima/ima_main.c | 5 ++++- security/integrity/ima/ima_policy.c | 6 +++++- security/integrity/integrity.h | 1 + 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index f1c5cc9..4c3efe4 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -23,7 +23,7 @@ Description: [fowner]] lsm: [[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] - option: [[appraise_type=]] + option: [[appraise_type=]] [permit_directio] base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK] mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC] diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index ba9e4d7..d719978 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -199,6 +199,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, struct evm_ima_xattr_data **xattr_value, int *xattr_len) { + const char *audit_cause = "failed"; struct inode *inode = file_inode(file); const char *filename = file->f_dentry->d_name.name; int result = 0; @@ -213,6 +214,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, if (!(iint->flags & IMA_COLLECTED)) { u64 i_version = file_inode(file)->i_version; + if (file->f_flags & O_DIRECT) { + audit_cause = "failed(directio)"; + result = -EACCES; + goto out; + } + /* use default hash algorithm */ hash.hdr.algo = ima_hash_algo; @@ -233,9 +240,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, result = -ENOMEM; } } +out: if (result) integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, - filename, "collect_data", "failed", + filename, "collect_data", audit_cause, result, 0); return result; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 52ac6cf..27ab5df 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -214,8 +214,11 @@ static int process_measurement(struct file *file, const char *filename, xattr_ptr = &xattr_value; rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len); - if (rc != 0) + if (rc != 0) { + if (file->f_flags & O_DIRECT) + rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; goto out_digsig; + } pathname = filename ?: ima_d_path(&file->f_path, &pathbuf); diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 93873a4..40a7488 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -353,7 +353,7 @@ enum { Opt_obj_user, Opt_obj_role, Opt_obj_type, Opt_subj_user, Opt_subj_role, Opt_subj_type, Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner, - Opt_appraise_type, Opt_fsuuid + Opt_appraise_type, Opt_fsuuid, Opt_permit_directio }; static match_table_t policy_tokens = { @@ -375,6 +375,7 @@ static match_table_t policy_tokens = { {Opt_uid, "uid=%s"}, {Opt_fowner, "fowner=%s"}, {Opt_appraise_type, "appraise_type=%s"}, + {Opt_permit_directio, "permit_directio"}, {Opt_err, NULL} }; @@ -622,6 +623,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else result = -EINVAL; break; + case Opt_permit_directio: + entry->flags |= IMA_PERMIT_DIRECTIO; + break; case Opt_err: ima_log_string(ab, "UNKNOWN", p); result = -EINVAL; diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 2fb5e53..33c0a70 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -30,6 +30,7 @@ #define IMA_ACTION_FLAGS 0xff000000 #define IMA_DIGSIG 0x01000000 #define IMA_DIGSIG_REQUIRED 0x02000000 +#define IMA_PERMIT_DIRECTIO 0x04000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_APPRAISE_SUBMASK) -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] ima: audit log files opened with O_DIRECT flag 2014-05-19 21:06 ` [PATCH v1] " Mimi Zohar @ 2014-05-21 9:38 ` Dmitry Kasatkin 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Kasatkin @ 2014-05-21 9:38 UTC (permalink / raw) To: Mimi Zohar, J. R. Okajima; +Cc: linux-security-module, Al Viro, linux-kernel On 20/05/14 06:06, Mimi Zohar wrote: > Files are measured or appraised based on the IMA policy. When a > file, in policy, is opened with the O_DIRECT flag, a deadlock > occurs. > > The first attempt at resolving this lockdep temporarily removed the > O_DIRECT flag and restored it, after calculating the hash. The > second attempt introduced the O_DIRECT_HAVELOCK flag. Based on this > flag, do_blockdev_direct_IO() would skip taking the i_mutex a second > time. The third attempt, by Dmitry Kasatkin, resolves the i_mutex > locking issue, by re-introducing the IMA mutex, but uncovered > another problem. Reading a file with O_DIRECT flag set, writes > directly to userspace pages. A second patch allocates a user-space > like memory. This works for all IMA hooks, except ima_file_free(), > which is called on __fput() to recalculate the file hash. > > Until this last issue is addressed, do not 'collect' the > measurement for measuring, appraising, or auditing files opened > with the O_DIRECT flag set. Based on policy, permit or deny file > access. This patch defines a new IMA policy rule option named > 'permit_directio'. Policy rules could be defined, based on LSM > or other criteria, to permit specific applications to open files > with the O_DIRECT flag set. > > Changelog v1: > - permit or deny file access based IMA policy rules > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> Acked-by: Dmitry Kasatkin <d.kasatkin@samsung.com> > --- > Documentation/ABI/testing/ima_policy | 2 +- > security/integrity/ima/ima_api.c | 10 +++++++++- > security/integrity/ima/ima_main.c | 5 ++++- > security/integrity/ima/ima_policy.c | 6 +++++- > security/integrity/integrity.h | 1 + > 5 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index f1c5cc9..4c3efe4 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -23,7 +23,7 @@ Description: > [fowner]] > lsm: [[subj_user=] [subj_role=] [subj_type=] > [obj_user=] [obj_role=] [obj_type=]] > - option: [[appraise_type=]] > + option: [[appraise_type=]] [permit_directio] > > base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK] > mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC] > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index ba9e4d7..d719978 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -199,6 +199,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > struct evm_ima_xattr_data **xattr_value, > int *xattr_len) > { > + const char *audit_cause = "failed"; > struct inode *inode = file_inode(file); > const char *filename = file->f_dentry->d_name.name; > int result = 0; > @@ -213,6 +214,12 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > if (!(iint->flags & IMA_COLLECTED)) { > u64 i_version = file_inode(file)->i_version; > > + if (file->f_flags & O_DIRECT) { > + audit_cause = "failed(directio)"; > + result = -EACCES; > + goto out; > + } > + > /* use default hash algorithm */ > hash.hdr.algo = ima_hash_algo; > > @@ -233,9 +240,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > result = -ENOMEM; > } > } > +out: > if (result) > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, > - filename, "collect_data", "failed", > + filename, "collect_data", audit_cause, > result, 0); > return result; > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 52ac6cf..27ab5df 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -214,8 +214,11 @@ static int process_measurement(struct file *file, const char *filename, > xattr_ptr = &xattr_value; > > rc = ima_collect_measurement(iint, file, xattr_ptr, &xattr_len); > - if (rc != 0) > + if (rc != 0) { > + if (file->f_flags & O_DIRECT) > + rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; > goto out_digsig; > + } > > pathname = filename ?: ima_d_path(&file->f_path, &pathbuf); > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 93873a4..40a7488 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -353,7 +353,7 @@ enum { > Opt_obj_user, Opt_obj_role, Opt_obj_type, > Opt_subj_user, Opt_subj_role, Opt_subj_type, > Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner, > - Opt_appraise_type, Opt_fsuuid > + Opt_appraise_type, Opt_fsuuid, Opt_permit_directio > }; > > static match_table_t policy_tokens = { > @@ -375,6 +375,7 @@ static match_table_t policy_tokens = { > {Opt_uid, "uid=%s"}, > {Opt_fowner, "fowner=%s"}, > {Opt_appraise_type, "appraise_type=%s"}, > + {Opt_permit_directio, "permit_directio"}, > {Opt_err, NULL} > }; > > @@ -622,6 +623,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > else > result = -EINVAL; > break; > + case Opt_permit_directio: > + entry->flags |= IMA_PERMIT_DIRECTIO; > + break; > case Opt_err: > ima_log_string(ab, "UNKNOWN", p); > result = -EINVAL; > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 2fb5e53..33c0a70 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -30,6 +30,7 @@ > #define IMA_ACTION_FLAGS 0xff000000 > #define IMA_DIGSIG 0x01000000 > #define IMA_DIGSIG_REQUIRED 0x02000000 > +#define IMA_PERMIT_DIRECTIO 0x04000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ > IMA_APPRAISE_SUBMASK) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-21 9:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-12 20:24 [RFC PATCH] ima: audit log files opened with O_DIRECT flag Mimi Zohar 2014-05-12 23:27 ` Mimi Zohar 2014-05-13 7:19 ` J. R. Okajima 2014-05-13 11:10 ` Mimi Zohar 2014-05-19 21:06 ` [PATCH v1] " Mimi Zohar 2014-05-21 9:38 ` Dmitry Kasatkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox