Menu
▾
▴
linux-ima-devel
|
From: James M. <jm...@na...> - 2017-08-16 09:52:31
|
On Wed, 16 Aug 2017, Christoph Hellwig wrote: > On Wed, Aug 16, 2017 at 12:43:58PM +1000, James Morris wrote: > > On Tue, 15 Aug 2017, Mimi Zohar wrote: > > > > > To resolve this locking problem, this patch set introduces a new > > > ->integrity_read file operation method. Until all filesystems > > > define the new ->integrity_read method, files that were previously > > > measured might not be currently measured and files that were > > > previously appraised might fail to be appraised properly. > > > > Are there any such filesystems in mainline which are not getting an > > integrity_read method in this patchset? > > There are a few, mostly because we're pretty sure the previous integrity > code did the wrong thing for them - e.g. ocfs2 and gfs2 where locking > vs operations on other cluster nodes was missing, or NFS where in addition > to the above deadlocks were 100% reprodicible with current code. Should we do a warn_once for these filesystems when IMA is used? -- James Morris <jm...@na...> |
|
From: Mimi Z. <zo...@li...> - 2017-08-16 11:05:53
|
On Wed, 2017-08-16 at 19:52 +1000, James Morris wrote: > On Wed, 16 Aug 2017, Christoph Hellwig wrote: > > > On Wed, Aug 16, 2017 at 12:43:58PM +1000, James Morris wrote: > > > On Tue, 15 Aug 2017, Mimi Zohar wrote: > > > > > > > To resolve this locking problem, this patch set introduces a new > > > > ->integrity_read file operation method. Until all filesystems > > > > define the new ->integrity_read method, files that were previously > > > > measured might not be currently measured and files that were > > > > previously appraised might fail to be appraised properly. > > > > > > Are there any such filesystems in mainline which are not getting an > > > integrity_read method in this patchset? > > > > There are a few, mostly because we're pretty sure the previous integrity > > code did the wrong thing for them - e.g. ocfs2 and gfs2 where locking > > vs operations on other cluster nodes was missing, or NFS where in addition > > to the above deadlocks were 100% reprodicible with current code. > > Should we do a warn_once for these filesystems when IMA is used? I don't think it is necessary. In terms of IMA-measurement, any file in policy on an unsupported filesystem will be in the measurement list, but the file hash will be 0's. In terms of IMA-appraisal, any file in policy on an unsupported filesystem will fail appraisal, since the file hash is 0. A separate patch set will emit a warning when a file system is mounted without i_version. Mimi |
|
From: Jan K. <ja...@su...> - 2017-08-16 13:18:01
|
On Tue 15-08-17 10:43:55, Mimi Zohar wrote: > From: Christoph Hellwig <hc...@ls...> > > Add a new ->integrity_read file operation to read data for integrity > hash collection. This is defined to be equivalent to ->read_iter, > except that it will be called with the i_rwsem held exclusively. > > (Based on Christoph's original patch.) > > Signed-off-by: Christoph Hellwig <hc...@ls...> > Cc: Matthew Garrett <mj...@sr...> > Cc: Jan Kara <ja...@su...> > Cc: "Theodore Ts'o" <ty...@mi...> > Cc: Andreas Dilger <adi...@di...> > Cc: Jaegeuk Kim <ja...@ke...> > Cc: Chao Yu <yu...@hu...> > Cc: Steven Whitehouse <swh...@re...> > Cc: Bob Peterson <rpe...@re...> > Cc: David Woodhouse <dw...@in...> > Cc: Dave Kleikamp <sh...@ke...> > Cc: Ryusuke Konishi <kon...@la...> > Cc: Mark Fasheh <mf...@ve...> > Cc: Joel Becker <jl...@ev...> > Cc: Richard Weinberger <ri...@no...> > Cc: "Darrick J. Wong" <dar...@or...> > Cc: Hugh Dickins <hu...@go...> > Cc: Chris Mason <cl...@fb...> > Signed-off-by: Mimi Zohar <zo...@li...> Looks good to me. You can add: Reviewed-by: Jan Kara <ja...@su...> Honza -- Jan Kara <ja...@su...> SUSE Labs, CR |
|
From: Mimi Z. <zo...@li...> - 2017-08-16 17:44:09
|
On Wed, 2017-08-16 at 15:17 +0200, Jan Kara wrote: > On Tue 15-08-17 10:43:55, Mimi Zohar wrote: > > From: Christoph Hellwig <hc...@ls...> > > > > Add a new ->integrity_read file operation to read data for integrity > > hash collection. This is defined to be equivalent to ->read_iter, > > except that it will be called with the i_rwsem held exclusively. > > > > (Based on Christoph's original patch.) > > > > Signed-off-by: Christoph Hellwig <hc...@ls...> > > Cc: Matthew Garrett <mj...@sr...> > > Cc: Jan Kara <ja...@su...> > > Cc: "Theodore Ts'o" <ty...@mi...> > > Cc: Andreas Dilger <adi...@di...> > > Cc: Jaegeuk Kim <ja...@ke...> > > Cc: Chao Yu <yu...@hu...> > > Cc: Steven Whitehouse <swh...@re...> > > Cc: Bob Peterson <rpe...@re...> > > Cc: David Woodhouse <dw...@in...> > > Cc: Dave Kleikamp <sh...@ke...> > > Cc: Ryusuke Konishi <kon...@la...> > > Cc: Mark Fasheh <mf...@ve...> > > Cc: Joel Becker <jl...@ev...> > > Cc: Richard Weinberger <ri...@no...> > > Cc: "Darrick J. Wong" <dar...@or...> > > Cc: Hugh Dickins <hu...@go...> > > Cc: Chris Mason <cl...@fb...> > > Signed-off-by: Mimi Zohar <zo...@li...> > > Looks good to me. You can add: > > Reviewed-by: Jan Kara <ja...@su...> Thanks! |
|
From: Mimi Z. <zo...@li...> - 2017-08-16 17:43:25
|
On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote: > Looks good, > > Reviewed-by: Christoph Hellwig <hc...@ls...> Thank you for the reviewed-by's. Up to now I haven't been removing the Changelog before sending James a pull request. Adding the dashes in the commit itself, only changes how the patches are applied by others to their local branch, not to what would be upstreamed. Am I suppose to be removing the changelog before sending a pull request? thanks, Mimi |
|
From: James M. <jm...@na...> - 2017-08-17 02:42:28
|
On Wed, 16 Aug 2017, Mimi Zohar wrote: > On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote: > > Looks good, > > > > Reviewed-by: Christoph Hellwig <hc...@ls...> > > Thank you for the reviewed-by's. > > Up to now I haven't been removing the Changelog before sending James a > pull request. Adding the dashes in the commit itself, only changes > how the patches are applied by others to their local branch, not to > what would be upstreamed. Am I suppose to be removing the changelog > before sending a pull request? I don't really understand this, but leave the changelog in? -- James Morris <jm...@na...> |
|
From: Mimi Z. <zo...@li...> - 2017-08-17 11:01:55
|
On Thu, 2017-08-17 at 12:42 +1000, James Morris wrote: > On Wed, 16 Aug 2017, Mimi Zohar wrote: > > > On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote: > > > Looks good, > > > > > > Reviewed-by: Christoph Hellwig <hc...@ls...> > > > > Thank you for the reviewed-by's. > > > > Up to now I haven't been removing the Changelog before sending James a > > pull request. Adding the dashes in the commit itself, only changes > > how the patches are applied by others to their local branch, not to > > what would be upstreamed. Am I suppose to be removing the changelog > > before sending a pull request? > > I don't really understand this, but leave the changelog in? Christoph had commented that the "---" should be before the Changelog, not afterwards, so that git-am skips them. The changelog is needed by reviewers, but once upstreamed it isn't really needed any more. Based on Christph's response (offline), any information in the changelog that is still useful, should move into the commit description itself or code. Mimi |
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:06:06
|
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> All files matching a "measure" rule must be included in the IMA
> measurement list, even when the file hash cannot be calculated.
> Similarly, all files matching an "audit" rule must be audited, even when
> the file hash can not be calculated.
>
> The file data hash field contained in the IMA measurement list template
> data will contain 0's instead of the actual file hash digest.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v6:
> - replace "?:" with if/then
> - annotate i_version usage
> - reword O_DIRECT comment
>
> Changelog v5:
> - Fail files opened O_DIRECT, but include attempt in measurement list.
>
> Changelog v4:
> - Based on both -EBADF and -EINVAL
> - clean up ima_collect_measurement()
>
> security/integrity/ima/ima_api.c | 67 +++++++++++++++++++++++--------------
> security/integrity/ima/ima_crypto.c | 10 ++++++
> security/integrity/ima/ima_main.c | 7 ++--
> 3 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c2edba8de35e..1dee695642a4 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -199,42 +199,59 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct inode *inode = file_inode(file);
> const char *filename = file->f_path.dentry->d_name.name;
> int result = 0;
> + int length;
> + void *tmpbuf;
> + u64 i_version;
> struct {
> struct ima_digest_data hdr;
> char digest[IMA_MAX_DIGEST_SIZE];
> } hash;
>
> - if (!(iint->flags & IMA_COLLECTED)) {
> - u64 i_version = file_inode(file)->i_version;
> + if (iint->flags & IMA_COLLECTED)
> + goto out;
>
> - if (file->f_flags & O_DIRECT) {
> - audit_cause = "failed(directio)";
> - result = -EACCES;
> - goto out;
> - }
> + /*
> + * Dectecting file change is based on i_version. On filesystems
> + * which do not support i_version, support is limited to an initial
> + * measurement/appraisal/audit.
> + */
> + i_version = file_inode(file)->i_version;
> + hash.hdr.algo = algo;
>
> - hash.hdr.algo = algo;
> -
> - result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> - ima_calc_buffer_hash(buf, size, &hash.hdr);
> - if (!result) {
> - int length = sizeof(hash.hdr) + hash.hdr.length;
> - void *tmpbuf = krealloc(iint->ima_hash, length,
> - GFP_NOFS);
> - if (tmpbuf) {
> - iint->ima_hash = tmpbuf;
> - memcpy(iint->ima_hash, &hash, length);
> - iint->version = i_version;
> - iint->flags |= IMA_COLLECTED;
> - } else
> - result = -ENOMEM;
> - }
> + /* Initialize hash digest to 0's in case of failure */
> + memset(&hash.digest, 0, sizeof(hash.digest));
> +
> + if (buf)
> + result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> + else
> + result = ima_calc_file_hash(file, &hash.hdr);
> +
> + if (result && result != -EBADF && result != -EINVAL)
> + goto out;
> +
> + length = sizeof(hash.hdr) + hash.hdr.length;
> + tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
> + if (!tmpbuf) {
> + result = -ENOMEM;
> + goto out;
> }
> +
> + iint->ima_hash = tmpbuf;
> + memcpy(iint->ima_hash, &hash, length);
> + iint->version = i_version;
> +
> + /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> + if (result != -EBADF && result != -EINVAL)
> + iint->flags |= IMA_COLLECTED;
Result can be other than 0, EBADF and EINVAL here?
It is confusing.. simpler than can be just
if (!result)
iint->flags |= IMA_COLLECTED;
Isn't it?
> out:
> - if (result)
> + if (result) {
> + if (file->f_flags & O_DIRECT)
> + audit_cause = "failed(directio)";
> +
> integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> filename, "collect_data", audit_cause,
> result, 0);
> + }
> return result;
> }
>
> @@ -278,7 +295,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
> }
>
> result = ima_store_template(entry, violation, inode, filename, pcr);
> - if (!result || result == -EEXIST) {
> + if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) {
> iint->flags |= IMA_MEASURED;
> iint->measured_pcrs |= (0x1 << pcr);
> }
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 802d5d20f36f..a856d8c9c9f3 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -441,6 +441,16 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> loff_t i_size;
> int rc;
>
> + /*
> + * For consistency, fail file's opened with the O_DIRECT flag on
> + * filesystems mounted with/without DAX option.
> + */
> + if (file->f_flags & O_DIRECT) {
> + hash->length = hash_digest_size[ima_hash_algo];
> + hash->algo = ima_hash_algo;
> + return -EINVAL;
> + }
> +
> i_size = i_size_read(file_inode(file));
>
> if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..d23dfe6ede18 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -235,11 +235,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>
> rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
> - if (rc != 0) {
> - if (file->f_flags & O_DIRECT)
> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
> + if (rc != 0 && rc != -EBADF && rc != -EINVAL)
> goto out_digsig;
> - }
>
> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
> pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> @@ -247,7 +244,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname,
> xattr_value, xattr_len, pcr);
> - if (action & IMA_APPRAISE_SUBMASK)
> + if (rc == 0 && (action & IMA_APPRAISE_SUBMASK))
> rc = ima_appraise_measurement(func, iint, file, pathname,
> xattr_value, xattr_len, opened);
> if (action & IMA_AUDIT)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 12:54:42
|
On Tue, 2017-08-22 at 13:05 +0300, Dmitry Kasatkin wrote:
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > All files matching a "measure" rule must be included in the IMA
> > measurement list, even when the file hash cannot be calculated.
> > Similarly, all files matching an "audit" rule must be audited, even when
> > the file hash can not be calculated.
> >
> > The file data hash field contained in the IMA measurement list template
> > data will contain 0's instead of the actual file hash digest.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v6:
> > - replace "?:" with if/then
> > - annotate i_version usage
> > - reword O_DIRECT comment
> >
> > Changelog v5:
> > - Fail files opened O_DIRECT, but include attempt in measurement list.
> >
> > Changelog v4:
> > - Based on both -EBADF and -EINVAL
> > - clean up ima_collect_measurement()
> >
> > security/integrity/ima/ima_api.c | 67 +++++++++++++++++++++++--------------
> > security/integrity/ima/ima_crypto.c | 10 ++++++
> > security/integrity/ima/ima_main.c | 7 ++--
> > 3 files changed, 54 insertions(+), 30 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c2edba8de35e..1dee695642a4 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -199,42 +199,59 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > struct inode *inode = file_inode(file);
> > const char *filename = file->f_path.dentry->d_name.name;
> > int result = 0;
> > + int length;
> > + void *tmpbuf;
> > + u64 i_version;
> > struct {
> > struct ima_digest_data hdr;
> > char digest[IMA_MAX_DIGEST_SIZE];
> > } hash;
> >
> > - if (!(iint->flags & IMA_COLLECTED)) {
> > - u64 i_version = file_inode(file)->i_version;
> > + if (iint->flags & IMA_COLLECTED)
> > + goto out;
> >
> > - if (file->f_flags & O_DIRECT) {
> > - audit_cause = "failed(directio)";
> > - result = -EACCES;
> > - goto out;
> > - }
> > + /*
> > + * Dectecting file change is based on i_version. On filesystems
> > + * which do not support i_version, support is limited to an initial
> > + * measurement/appraisal/audit.
> > + */
> > + i_version = file_inode(file)->i_version;
> > + hash.hdr.algo = algo;
> >
> > - hash.hdr.algo = algo;
> > -
> > - result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
> > - ima_calc_buffer_hash(buf, size, &hash.hdr);
> > - if (!result) {
> > - int length = sizeof(hash.hdr) + hash.hdr.length;
> > - void *tmpbuf = krealloc(iint->ima_hash, length,
> > - GFP_NOFS);
> > - if (tmpbuf) {
> > - iint->ima_hash = tmpbuf;
> > - memcpy(iint->ima_hash, &hash, length);
> > - iint->version = i_version;
> > - iint->flags |= IMA_COLLECTED;
> > - } else
> > - result = -ENOMEM;
> > - }
> > + /* Initialize hash digest to 0's in case of failure */
> > + memset(&hash.digest, 0, sizeof(hash.digest));
> > +
> > + if (buf)
> > + result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> > + else
> > + result = ima_calc_file_hash(file, &hash.hdr);
> > +
> > + if (result && result != -EBADF && result != -EINVAL)
> > + goto out;
> > +
> > + length = sizeof(hash.hdr) + hash.hdr.length;
> > + tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
> > + if (!tmpbuf) {
> > + result = -ENOMEM;
> > + goto out;
> > }
> > +
> > + iint->ima_hash = tmpbuf;
> > + memcpy(iint->ima_hash, &hash, length);
> > + iint->version = i_version;
> > +
> > + /* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> > + if (result != -EBADF && result != -EINVAL)
> > + iint->flags |= IMA_COLLECTED;
>
>
> Result can be other than 0, EBADF and EINVAL here?
> It is confusing.. simpler than can be just
>
> if (!result)
> iint->flags |= IMA_COLLECTED;
>
> Isn't it?
Yes, that is better.
Mimi
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:07:28
|
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines the "dont_failsafe"
> policy action rule.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
> - fail attempt to add dont_failsafe rule when appending to the policy
>
> Documentation/ABI/testing/ima_policy | 3 ++-
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_main.c | 12 +++++++++++-
> security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
> 4 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index e76432b9954d..f271207743e5 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -17,7 +17,8 @@ Description:
>
> rule format: action [condition ...]
>
> - action: measure | dont_measure | appraise | dont_appraise | audit
> + action: measure | dont_meaure | appraise | dont_appraise |
> + audit | dont_failsafe
> condition:= base | lsm [option]
> base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> [euid=] [fowner=]]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d52b487ad259..c5f34f7c5b0f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> void ima_policy_stop(struct seq_file *m, void *v);
> int ima_policy_show(struct seq_file *m, void *v);
> +void set_failsafe(bool flag);
>
> /* Appraise integrity measurements */
> #define IMA_APPRAISE_ENFORCE 0x01
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d23dfe6ede18..b00186914df8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -38,6 +38,12 @@ int ima_appraise;
> int ima_hash_algo = HASH_ALGO_SHA1;
> static int hash_setup_done;
>
> +static bool ima_failsafe = 1;
> +void set_failsafe(bool flag)
> +{
> + ima_failsafe = flag;
> +}
> +
> static int __init hash_setup(char *str)
> {
> struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> __putname(pathbuf);
> out:
> inode_unlock(inode);
> - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + if (!ima_failsafe && rc == -EBADF)
> + return 0;
> +
By default IMA is failsafe. ima_failsafe is true.
Return 0 is needed in failsafe mode. right?
But in this logic it will happen if ima_failsafe is false. meaning it
is not failsafe.
Is it a typo?
> return -EACCES;
> + }
> return 0;
> }
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 95209a5f8595..43b85a4fb8e8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -40,12 +40,14 @@
> #define APPRAISE 0x0004 /* same as IMA_APPRAISE */
> #define DONT_APPRAISE 0x0008
> #define AUDIT 0x0040
> +#define DONT_FAILSAFE 0x0400
>
> #define INVALID_PCR(a) (((a) < 0) || \
> (a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
>
> int ima_policy_flag;
> static int temp_ima_appraise;
> +static bool temp_failsafe = 1;
>
> #define MAX_LSM_RULES 6
> enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
> @@ -513,6 +515,9 @@ void ima_update_policy(void)
> if (ima_rules != policy) {
> ima_policy_flag = 0;
> ima_rules = policy;
> +
> + /* Only update on initial policy replacement, not append */
> + set_failsafe(temp_failsafe);
> }
> ima_update_policy_flag();
> }
> @@ -529,7 +534,7 @@ enum {
> Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> Opt_appraise_type, Opt_permit_directio,
> - Opt_pcr
> + Opt_pcr, Opt_dont_failsafe
> };
>
> static match_table_t policy_tokens = {
> @@ -560,6 +565,7 @@ static match_table_t policy_tokens = {
> {Opt_appraise_type, "appraise_type=%s"},
> {Opt_permit_directio, "permit_directio"},
> {Opt_pcr, "pcr=%s"},
> + {Opt_dont_failsafe, "dont_failsafe"},
> {Opt_err, NULL}
> };
>
> @@ -630,6 +636,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
> continue;
> token = match_token(p, policy_tokens, args);
> + if (entry->action == DONT_FAILSAFE) {
> + /* no args permitted, force invalid rule */
> + token = Opt_dont_failsafe;
> + }
> +
> switch (token) {
> case Opt_measure:
> ima_log_string(ab, "action", "measure");
> @@ -671,6 +682,19 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>
> entry->action = AUDIT;
> break;
> + case Opt_dont_failsafe:
> + ima_log_string(ab, "action", "dont_failsafe");
> +
> + if (entry->action != UNKNOWN)
> + result = -EINVAL;
> +
> + /* Permit on initial policy replacement only */
> + if (ima_rules != &ima_policy_rules)
> + temp_failsafe = 0;
> + else
> + result = -EINVAL;
> + entry->action = DONT_FAILSAFE;
> + break;
> case Opt_func:
> ima_log_string(ab, "func", args[0].from);
>
> @@ -949,6 +973,7 @@ void ima_delete_rules(void)
> int i;
>
> temp_ima_appraise = 0;
> + temp_failsafe = 1;
> list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
> for (i = 0; i < MAX_LSM_RULES; i++)
> kfree(entry->lsm[i].args_p);
> @@ -1040,6 +1065,8 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_puts(m, pt(Opt_dont_appraise));
> if (entry->action & AUDIT)
> seq_puts(m, pt(Opt_audit));
> + if (entry->action & DONT_FAILSAFE)
> + seq_puts(m, pt(Opt_dont_failsafe));
>
> seq_puts(m, " ");
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 10:07:58
|
Looks good to me.
On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> Permit normally denied access/execute permission for files in policy
> on IMA unsupported filesystems. This patch defines "fs_unsafe", a
> builtin policy.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
>
> ---
> Changelog v3:
> - include dont_failsafe rule when displaying policy
>
> Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
> security/integrity/ima/ima_policy.c | 12 ++++++++++++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d9c171ce4190..4e303be83df6 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1502,7 +1502,7 @@
>
> ima_policy= [IMA]
> The builtin policies to load during IMA setup.
> - Format: "tcb | appraise_tcb | secure_boot"
> + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
>
> The "tcb" policy measures all programs exec'd, files
> mmap'd for exec, and all files opened with the read
> @@ -1517,6 +1517,12 @@
> of files (eg. kexec kernel image, kernel modules,
> firmware, policy, etc) based on file signatures.
>
> + The "fs_unsafe" policy permits normally denied
> + access/execute permission for files in policy on IMA
> + unsupported filesystems. Note this option, as the
> + name implies, is not safe and not recommended for
> + any environments other than testing.
> +
> ima_tcb [IMA] Deprecated. Use ima_policy= instead.
> Load a policy which meets the needs of the Trusted
> Computing Base. This means IMA will measure all
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 43b85a4fb8e8..cddd9dfb01e1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
> .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> };
>
> +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
> + {.action = DONT_FAILSAFE}
> +};
> +
> static LIST_HEAD(ima_default_rules);
> static LIST_HEAD(ima_policy_rules);
> static LIST_HEAD(ima_temp_rules);
> @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>
> static bool ima_use_appraise_tcb __initdata;
> static bool ima_use_secure_boot __initdata;
> +static bool ima_use_dont_failsafe __initdata;
> static int __init policy_setup(char *str)
> {
> char *p;
> @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
> ima_use_appraise_tcb = 1;
> else if (strcmp(p, "secure_boot") == 0)
> ima_use_secure_boot = 1;
> + else if (strcmp(p, "fs_unsafe") == 0) {
> + ima_use_dont_failsafe = 1;
> + set_failsafe(0);
> + }
> }
>
> return 1;
> @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
> temp_ima_appraise |= IMA_APPRAISE_POLICY;
> }
>
> + if (ima_use_dont_failsafe)
> + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
> +
> ima_rules = &ima_default_rules;
> ima_update_policy_flag();
> }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 12:55:11
|
On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > Permit normally denied access/execute permission for files in policy
> > on IMA unsupported filesystems. This patch defines the "dont_failsafe"
> > policy action rule.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v3:
> > - include dont_failsafe rule when displaying policy
> > - fail attempt to add dont_failsafe rule when appending to the policy
> >
> > Documentation/ABI/testing/ima_policy | 3 ++-
> > security/integrity/ima/ima.h | 1 +
> > security/integrity/ima/ima_main.c | 12 +++++++++++-
> > security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
> > 4 files changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index e76432b9954d..f271207743e5 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -17,7 +17,8 @@ Description:
> >
> > rule format: action [condition ...]
> >
> > - action: measure | dont_measure | appraise | dont_appraise | audit
> > + action: measure | dont_meaure | appraise | dont_appraise |
> > + audit | dont_failsafe
> > condition:= base | lsm [option]
> > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
> > [euid=] [fowner=]]
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index d52b487ad259..c5f34f7c5b0f 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> > void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> > void ima_policy_stop(struct seq_file *m, void *v);
> > int ima_policy_show(struct seq_file *m, void *v);
> > +void set_failsafe(bool flag);
> >
> > /* Appraise integrity measurements */
> > #define IMA_APPRAISE_ENFORCE 0x01
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index d23dfe6ede18..b00186914df8 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -38,6 +38,12 @@ int ima_appraise;
> > int ima_hash_algo = HASH_ALGO_SHA1;
> > static int hash_setup_done;
> >
> > +static bool ima_failsafe = 1;
> > +void set_failsafe(bool flag)
> > +{
> > + ima_failsafe = flag;
> > +}
> > +
> > static int __init hash_setup(char *str)
> > {
> > struct ima_template_desc *template_desc = ima_template_desc_current();
> > @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> > __putname(pathbuf);
> > out:
> > inode_unlock(inode);
> > - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> > + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > + if (!ima_failsafe && rc == -EBADF)
> > + return 0;
> > +
>
> By default IMA is failsafe. ima_failsafe is true.
> Return 0 is needed in failsafe mode. right?
> But in this logic it will happen if ima_failsafe is false. meaning it
> is not failsafe.
>
> Is it a typo?
No, the default, as you pointed out above, is failsafe mode. Only when we are not in failsafe mode, do we allow the file access/execute for file's that we could not appraise.
Mimi
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 13:31:14
|
On Tue, Aug 22, 2017 at 3:54 PM, Mimi Zohar <zo...@li...> wrote:
> On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
>> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
>> > Permit normally denied access/execute permission for files in policy
>> > on IMA unsupported filesystems. This patch defines the "dont_failsafe"
>> > policy action rule.
>> >
>> > Signed-off-by: Mimi Zohar <zo...@li...>
>> >
>> > ---
>> > Changelog v3:
>> > - include dont_failsafe rule when displaying policy
>> > - fail attempt to add dont_failsafe rule when appending to the policy
>> >
>> > Documentation/ABI/testing/ima_policy | 3 ++-
>> > security/integrity/ima/ima.h | 1 +
>> > security/integrity/ima/ima_main.c | 12 +++++++++++-
>> > security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
>> > 4 files changed, 42 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> > index e76432b9954d..f271207743e5 100644
>> > --- a/Documentation/ABI/testing/ima_policy
>> > +++ b/Documentation/ABI/testing/ima_policy
>> > @@ -17,7 +17,8 @@ Description:
>> >
>> > rule format: action [condition ...]
>> >
>> > - action: measure | dont_measure | appraise | dont_appraise | audit
>> > + action: measure | dont_meaure | appraise | dont_appraise |
>> > + audit | dont_failsafe
>> > condition:= base | lsm [option]
>> > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
>> > [euid=] [fowner=]]
>> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> > index d52b487ad259..c5f34f7c5b0f 100644
>> > --- a/security/integrity/ima/ima.h
>> > +++ b/security/integrity/ima/ima.h
>> > @@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
>> > void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>> > void ima_policy_stop(struct seq_file *m, void *v);
>> > int ima_policy_show(struct seq_file *m, void *v);
>> > +void set_failsafe(bool flag);
>> >
>> > /* Appraise integrity measurements */
>> > #define IMA_APPRAISE_ENFORCE 0x01
>> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> > index d23dfe6ede18..b00186914df8 100644
>> > --- a/security/integrity/ima/ima_main.c
>> > +++ b/security/integrity/ima/ima_main.c
>> > @@ -38,6 +38,12 @@ int ima_appraise;
>> > int ima_hash_algo = HASH_ALGO_SHA1;
>> > static int hash_setup_done;
>> >
>> > +static bool ima_failsafe = 1;
>> > +void set_failsafe(bool flag)
>> > +{
>> > + ima_failsafe = flag;
>> > +}
>> > +
>> > static int __init hash_setup(char *str)
>> > {
>> > struct ima_template_desc *template_desc = ima_template_desc_current();
>> > @@ -260,8 +266,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>> > __putname(pathbuf);
>> > out:
>> > inode_unlock(inode);
>> > - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
>> > + if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> > + if (!ima_failsafe && rc == -EBADF)
>> > + return 0;
>> > +
>>
>> By default IMA is failsafe. ima_failsafe is true.
>> Return 0 is needed in failsafe mode. right?
>> But in this logic it will happen if ima_failsafe is false. meaning it
>> is not failsafe.
>>
>> Is it a typo?
>
> No, the default, as you pointed out above, is failsafe mode. Only when we are not in failsafe mode, do we allow the file access/execute for file's that we could not appraise.
>
> Mimi
>
So in your language "failsafe" means IMA must fail/return with error
on failure..
Ok. then logic is correct and OK with me.
--
Thanks,
Dmitry
|
|
From: Mimi Z. <zo...@li...> - 2017-08-22 13:14:03
|
On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
> Looks good to me.
Thank you for reviewing the code! Can I add your Reviewed-by/Acked-by?
Mimi
>
> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
> > Permit normally denied access/execute permission for files in policy
> > on IMA unsupported filesystems. This patch defines "fs_unsafe", a
> > builtin policy.
> >
> > Signed-off-by: Mimi Zohar <zo...@li...>
> >
> > ---
> > Changelog v3:
> > - include dont_failsafe rule when displaying policy
> >
> > Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
> > security/integrity/ima/ima_policy.c | 12 ++++++++++++
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index d9c171ce4190..4e303be83df6 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1502,7 +1502,7 @@
> >
> > ima_policy= [IMA]
> > The builtin policies to load during IMA setup.
> > - Format: "tcb | appraise_tcb | secure_boot"
> > + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
> >
> > The "tcb" policy measures all programs exec'd, files
> > mmap'd for exec, and all files opened with the read
> > @@ -1517,6 +1517,12 @@
> > of files (eg. kexec kernel image, kernel modules,
> > firmware, policy, etc) based on file signatures.
> >
> > + The "fs_unsafe" policy permits normally denied
> > + access/execute permission for files in policy on IMA
> > + unsupported filesystems. Note this option, as the
> > + name implies, is not safe and not recommended for
> > + any environments other than testing.
> > +
> > ima_tcb [IMA] Deprecated. Use ima_policy= instead.
> > Load a policy which meets the needs of the Trusted
> > Computing Base. This means IMA will measure all
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 43b85a4fb8e8..cddd9dfb01e1 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
> > .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
> > };
> >
> > +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
> > + {.action = DONT_FAILSAFE}
> > +};
> > +
> > static LIST_HEAD(ima_default_rules);
> > static LIST_HEAD(ima_policy_rules);
> > static LIST_HEAD(ima_temp_rules);
> > @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
> >
> > static bool ima_use_appraise_tcb __initdata;
> > static bool ima_use_secure_boot __initdata;
> > +static bool ima_use_dont_failsafe __initdata;
> > static int __init policy_setup(char *str)
> > {
> > char *p;
> > @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
> > ima_use_appraise_tcb = 1;
> > else if (strcmp(p, "secure_boot") == 0)
> > ima_use_secure_boot = 1;
> > + else if (strcmp(p, "fs_unsafe") == 0) {
> > + ima_use_dont_failsafe = 1;
> > + set_failsafe(0);
> > + }
> > }
> >
> > return 1;
> > @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
> > temp_ima_appraise |= IMA_APPRAISE_POLICY;
> > }
> >
> > + if (ima_use_dont_failsafe)
> > + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
> > +
> > ima_rules = &ima_default_rules;
> > ima_update_policy_flag();
> > }
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to maj...@vg...
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
|
|
From: Dmitry K. <dmi...@gm...> - 2017-08-22 13:41:16
|
Hi,
Yes, please add one of appropriate.
Thanks,
Dmitry
On Tue, Aug 22, 2017 at 4:13 PM, Mimi Zohar <zo...@li...> wrote:
> On Tue, 2017-08-22 at 13:07 +0300, Dmitry Kasatkin wrote:
>> Looks good to me.
>
> Thank you for reviewing the code! Can I add your Reviewed-by/Acked-by?
>
> Mimi
>
>>
>> On Tue, Aug 15, 2017 at 5:43 PM, Mimi Zohar <zo...@li...> wrote:
>> > Permit normally denied access/execute permission for files in policy
>> > on IMA unsupported filesystems. This patch defines "fs_unsafe", a
>> > builtin policy.
>> >
>> > Signed-off-by: Mimi Zohar <zo...@li...>
>> >
>> > ---
>> > Changelog v3:
>> > - include dont_failsafe rule when displaying policy
>> >
>> > Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
>> > security/integrity/ima/ima_policy.c | 12 ++++++++++++
>> > 2 files changed, 19 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> > index d9c171ce4190..4e303be83df6 100644
>> > --- a/Documentation/admin-guide/kernel-parameters.txt
>> > +++ b/Documentation/admin-guide/kernel-parameters.txt
>> > @@ -1502,7 +1502,7 @@
>> >
>> > ima_policy= [IMA]
>> > The builtin policies to load during IMA setup.
>> > - Format: "tcb | appraise_tcb | secure_boot"
>> > + Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
>> >
>> > The "tcb" policy measures all programs exec'd, files
>> > mmap'd for exec, and all files opened with the read
>> > @@ -1517,6 +1517,12 @@
>> > of files (eg. kexec kernel image, kernel modules,
>> > firmware, policy, etc) based on file signatures.
>> >
>> > + The "fs_unsafe" policy permits normally denied
>> > + access/execute permission for files in policy on IMA
>> > + unsupported filesystems. Note this option, as the
>> > + name implies, is not safe and not recommended for
>> > + any environments other than testing.
>> > +
>> > ima_tcb [IMA] Deprecated. Use ima_policy= instead.
>> > Load a policy which meets the needs of the Trusted
>> > Computing Base. This means IMA will measure all
>> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> > index 43b85a4fb8e8..cddd9dfb01e1 100644
>> > --- a/security/integrity/ima/ima_policy.c
>> > +++ b/security/integrity/ima/ima_policy.c
>> > @@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
>> > .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
>> > };
>> >
>> > +static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
>> > + {.action = DONT_FAILSAFE}
>> > +};
>> > +
>> > static LIST_HEAD(ima_default_rules);
>> > static LIST_HEAD(ima_policy_rules);
>> > static LIST_HEAD(ima_temp_rules);
>> > @@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
>> >
>> > static bool ima_use_appraise_tcb __initdata;
>> > static bool ima_use_secure_boot __initdata;
>> > +static bool ima_use_dont_failsafe __initdata;
>> > static int __init policy_setup(char *str)
>> > {
>> > char *p;
>> > @@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
>> > ima_use_appraise_tcb = 1;
>> > else if (strcmp(p, "secure_boot") == 0)
>> > ima_use_secure_boot = 1;
>> > + else if (strcmp(p, "fs_unsafe") == 0) {
>> > + ima_use_dont_failsafe = 1;
>> > + set_failsafe(0);
>> > + }
>> > }
>> >
>> > return 1;
>> > @@ -470,6 +479,9 @@ void __init ima_init_policy(void)
>> > temp_ima_appraise |= IMA_APPRAISE_POLICY;
>> > }
>> >
>> > + if (ima_use_dont_failsafe)
>> > + list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
>> > +
>> > ima_rules = &ima_default_rules;
>> > ima_update_policy_flag();
>> > }
>> > --
>> > 2.7.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> > the body of a message to maj...@vg...
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
--
Thanks,
Dmitry
|
|
From: Al V. <vi...@Ze...> - 2017-08-28 04:13:23
|
On Tue, Aug 15, 2017 at 10:43:55AM -0400, Mimi Zohar wrote: > From: Christoph Hellwig <hc...@ls...> > > Add a new ->integrity_read file operation to read data for integrity > hash collection. This is defined to be equivalent to ->read_iter, > except that it will be called with the i_rwsem held exclusively. Hmm... I'm really tempted to add default_integrity_read() that would just call ->read_iter(), with boilerplate part becoming .integrity_read = default_integrity_read Note that all stuff accessed in it would be fresh in caches, so it's not as if we had serious overhead there. And we are going to be reading from file, anyway... I agree that it should be an opt-in from filesystem; default is still "don't know how to read, sod off". It's just that telling at the glance whether it's supposed to be a simple case or something tricky is needed would be simpler that way and it might turn out to be more robust that way... |
|
From: Mimi Z. <zo...@li...> - 2017-08-28 18:30:51
|
On Mon, 2017-08-28 at 05:13 +0100, Al Viro wrote: > On Tue, Aug 15, 2017 at 10:43:55AM -0400, Mimi Zohar wrote: > > From: Christoph Hellwig <hc...@ls...> > > > > Add a new ->integrity_read file operation to read data for integrity > > hash collection. This is defined to be equivalent to ->read_iter, > > except that it will be called with the i_rwsem held exclusively. > > Hmm... I'm really tempted to add default_integrity_read() that would > just call ->read_iter(), with boilerplate part becoming > .integrity_read = default_integrity_read How can it automatically call the fs read_iter() without knowing if the fs read_iter() takes the i_rwsem? Or are you suggesting that the default_integrity_read is defined as generic_file_read_iter()? Mimi > Note that all stuff accessed in it would be fresh in caches, so > it's not as if we had serious overhead there. And we are going > to be reading from file, anyway... > > I agree that it should be an opt-in from filesystem; default is still > "don't know how to read, sod off". It's just that telling at the > glance whether it's supposed to be a simple case or something tricky > is needed would be simpler that way and it might turn out to be > more robust that way... > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to maj...@vg... > More majordomo info at http://vger.kernel.org/majordomo-info.html > |
Thanks for helping keep SourceForge clean.
X