linux-ima-devel — Discussion list for problems or development.

[Linux-ima-devel] [PATCH 2/3] tpm: retrieve digest size of unknown algorithms with PCR read

[Roberto S. <rob...@hu...>]

PCRs can be extended by providing the TPM algorithm identifier and
the digest. To correctly build the command buffer, the digest size
must be known.

The TPM driver cannot determine the digest size if the provided
TPM algorithm is not mapped to any crypto algorithm. In this case,
the PCR bank is not extended and could be used by attackers to protect
measurements made by themselves, which do not reflect the true status
of the platform.

To avoid this situation, the digest size of unknown algorithms is
determined at TPM initialization time with a PCR read, and stored
in the tpm_chip structure. The array of algorithms (active_banks)
has been replaced with an array of active_pcr_bank_info, a new structure
containing both the TPM algorithm identifier and the digest size.

Signed-off-by: Roberto Sassu <rob...@hu...>
---
 drivers/char/tpm/tpm-interface.c |  4 +--
 drivers/char/tpm/tpm.h           |  2 +-
 drivers/char/tpm/tpm2-cmd.c      | 55 ++++++++++++++++++++++++++++++++--------
 include/linux/tpm.h              |  5 ++++
 4 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1d6729b..2c3d973 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -914,8 +914,8 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 		memset(digest_list, 0, sizeof(digest_list));
 
 		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
-			digest_list[i].alg_id = chip->active_banks[i];
+		     chip->active_banks[i].alg_id != TPM2_ALG_ERROR; i++) {
+			digest_list[i].alg_id = chip->active_banks[i].alg_id;
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 			count++;
 		}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2d5466a..fb94bd2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -225,7 +225,7 @@ struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	struct active_bank_info active_banks[7];
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0cad0f6..b1356be 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -291,7 +291,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > ARRAY_SIZE(chip->active_banks))
 		return -EINVAL;
@@ -313,14 +312,10 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, count);
 
 	for (i = 0; i < count; i++) {
-		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
-			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
-				continue;
-			tpm_buf_append_u16(&buf, digests[i].alg_id);
-			tpm_buf_append(&buf, (const unsigned char
-					      *)&digests[i].digest,
-			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
-		}
+		/* digests[i].alg_id == chip->active_banks[i].alg_id */
+		tpm_buf_append_u16(&buf, digests[i].alg_id);
+		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+			       chip->active_banks[i].digest_size);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -943,6 +938,39 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_init_active_bank_info(struct tpm_chip *chip, u16 alg_id,
+				      struct active_bank_info *active_bank)
+{
+	struct tpm_buf buf;
+	struct tpm2_pcr_read_out *out;
+	int rc, i;
+
+	active_bank->alg_id = alg_id;
+
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
+
+		if (active_bank->alg_id != tpm2_hash_map[i].tpm_id)
+			continue;
+
+		active_bank->digest_size = hash_digest_size[crypto_algo];
+		return 0;
+	}
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
+	if (rc)
+		return rc;
+
+	rc = tpm2_pcr_read_common(chip, 0, alg_id, &buf, NULL);
+	if (rc == 0) {
+		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
+		active_bank->digest_size = be16_to_cpu(out->digest_size);
+	}
+
+	tpm_buf_destroy(&buf);
+	return 0;
+}
+
 struct tpm2_pcr_selection {
 	__be16  hash_alg;
 	u8  size_of_select;
@@ -997,7 +1025,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		}
 
 		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
-		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
+		rc =  tpm2_init_active_bank_info(chip,
+					be16_to_cpu(pcr_selection.hash_alg),
+					&chip->active_banks[i]);
+		if (rc)
+			break;
+
 		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
 			sizeof(pcr_selection.size_of_select) +
 			pcr_selection.size_of_select;
@@ -1006,7 +1039,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 
 out:
 	if (i < ARRAY_SIZE(chip->active_banks))
-		chip->active_banks[i] = TPM2_ALG_ERROR;
+		chip->active_banks[i].alg_id = TPM2_ALG_ERROR;
 
 	tpm_buf_destroy(&buf);
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5a090f5..3ecce21 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -52,6 +52,11 @@ struct tpm_class_ops {
 	void (*relinquish_locality)(struct tpm_chip *chip, int loc);
 };
 
+struct active_bank_info {
+	u16 alg_id;
+	u16 digest_size;
+};
+
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(u32 chip_num);
-- 
2.9.3

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Z. <zo...@li...>]

On Mon, 2017-09-18 at 10:55 -0400, Mimi Zohar wrote:
> On Mon, 2017-09-18 at 12:13 +0200, Jan Kara wrote:
> > On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> > > On 17/09/17 17:38, Al Viro wrote:
> > > >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> > > >>Now, I suspect most (all?) do, but that's a historical artifact rather
> > > >>than "design". In particular, the VFS layer used to do the locking for
> > > >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> > > >>that writes be seen atomically).
> > > >>
> > > >>But that lock was pushed down into the filesystems, since some
> > > >>filesystems really wanted to have parallel writes (particularly for
> > > >>direct IO, where that POSIX serialization requirement doesn't exist).
> > > >>
> > > >>That's all many years ago, though. New filesystems are likely to have
> > > >>copied the pattern from old ones, but even then..
> > > >>
> > > >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> > > >>as a lock. You can have the question of *which* inode gets talked
> > > >>about when you have things like eoverlayfs etc. Normally it would be
> > > >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> > > >>same thing in the simple cases), and sometimes it really wouldn't be
> > > >>obvious at all..
> > > >>
> > > >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> > > >>one of those things that are "mostly right for the simple cases",
> > > >>but...
> > > >The thing pretty much common to all of them is that write() might need
> > > >to modify permissions (suid removal), which brings ->i_rwsem in one
> > > >way or another - notify_change() needs that held...


> > > For GFS2, if we are to hold the inode info constant while it is checked, we
> > > would need to take a glock (read lock in this case) across the relevant
> > > operations. The glock will be happy under i_rwlock, since we have a lock
> > > ordering that takes local locks ahead of cluster locks. I've not dug into
> > > this enough to figure out whether the current proposal will allow this to
> > > work with GFS2 though. Does IMA cache the results from the
> > > ->read_integrity() operation?
> 
> Up to now, the hash calculation was stored in the iint structure,
> which is then used to extend the TPM, verify the file's integrity
> compared to the value stored in the xattr, and included in an audit
> message.
> 
> A new patch set by Thiago Bauermann will add appended signature
> support, re-using the kernel module signature appended method, which
> might require re-calculating the file hash based on a different hash
> algorithm.
> 
> > So I have asked Mimi about clustered filesystems before. And for now the
> > answer was that IMA for clustered filesystems is not supported (it will
> > return some error since ->integrity_read is NULL). If we would ever want to
> > support those it would require larger overhaul of the IMA architecture to
> > give filesystem more control over the locking (which is essentially what
> > Linus wants).
> 
> For performance reasons, IMA is not on a write hook, but detects file
> change on the last __fput() opened for write.  At that point, the
> cached info is reset.  The file hash is re-calculated and written out
> as an xattr.  On the next file access (in policy), the file hash is
> re-calculated and stored in the iint.
> 
> In terms of remote/clustered/fuse filesystems, we wouldn't be on the
> __fput() path.  Support for remote/clustered/fuse filesystems, would
> be similar to filesystems that do not support i_version.  Meaning only
> the first file access (in policy) would be measured/appraised, but not
> subsequent ones.  Even if we could detect file change, we would be
> dependent on the remote/clustered/fuse filesystem to inform us of the
> change.  What type of integrity guarantees would that provide?

After thinking this over a bit, perhaps we shouldn't cache the file
integrity results for these filesystems, since we can't rely on them
to notify us of a change (eg. malicious fs), but simply re-measure/re-
validate files each time.

Mimi

Re: [Linux-ima-devel] [PATCH] ima: Support filesystems without i_version support

[Sascha H. <s....@pe...>]

On Wed, Sep 20, 2017 at 08:06:27AM -0400, Mimi Zohar wrote:
> Hi Sascha,
> 
> On Wed, 2017-09-20 at 09:23 +0200, Sascha Hauer wrote:
> > Mimi,
> > 
> > On Wed, Sep 13, 2017 at 04:15:13PM +0200, Sascha Hauer wrote:
> > > IMA uses the inode's i_version field to detect changes on an inode.
> > > This seems to be an optimization for IMA and not strictly necessary.
> > > Just ignore the i_version field if it is zero and measure the file
> > > anyway. On filesystems which do not support i_version this may result
> > > in an unnecessary re-measurement of a file when it has been opened for
> > > writing without anything actually being written. For filesystems with
> > > i_version support the behaviour doesn't change.
> > > 
> > > Signed-off-by: Sascha Hauer <s....@pe...>
> > > ---
> > >  security/integrity/ima/ima_main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > >  I'm not sure if this patch is appropriate, but even when it's not it
> > >  would be interesting to know why it isn't.
> > 
> > Any input to this one?
> 
> Sorry, I'm still thinking about it.  For filesystems that
> automatically enable i_version there would be no difference.  For
> filesystems that require a mount option to enable i_version, this
> changes the behavior.
> 
> This is slightly different than not caching the integrity results, in
> that the cache is only cleared if someone opens the file rw.
> 
> (Jeff Layton posted a patch that replaces the i_version checks with
> atime/mtime.)

Looking at that patch I think that using i_version only when
MS_I_VERSION is set is a useful change because otherwise IMA
ends up using i_version when it contains no sensible values.

I can't judge whether mtime is fine grained enough on all systems,
but I don't think it's necessary to use it.

My version of ima_should_update_iint() would look like:

static bool ima_should_update_iint(struct integrity_iint_cache *iint,
				struct inode *inode)
{
	if (atomic_read(&inode->i_writecount) != 1)
		return false;
	if (iint->flags & IMA_NEW_FILE)
		return true;
	if (IS_I_VERSION(inode) && iint->version == inode->i_version)
		return false;
	return true;
}

That is, when we don't know for sure that an inode has not changed, we
must assume that it has changed and remeasure it. When in doubt we must
make sure IMA is working as expected, everything else is performance
optimization.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[SPAM] 尽日惹飞絮

[温琪同 <351...@qq...>]

An HTML attachment was scrubbed...

Re: [Linux-ima-devel] [PATCH] ima: Support filesystems without i_version support

[Mimi Z. <zo...@li...>]

Hi Sascha,

On Wed, 2017-09-20 at 09:23 +0200, Sascha Hauer wrote:
> Mimi,
> 
> On Wed, Sep 13, 2017 at 04:15:13PM +0200, Sascha Hauer wrote:
> > IMA uses the inode's i_version field to detect changes on an inode.
> > This seems to be an optimization for IMA and not strictly necessary.
> > Just ignore the i_version field if it is zero and measure the file
> > anyway. On filesystems which do not support i_version this may result
> > in an unnecessary re-measurement of a file when it has been opened for
> > writing without anything actually being written. For filesystems with
> > i_version support the behaviour doesn't change.
> > 
> > Signed-off-by: Sascha Hauer <s....@pe...>
> > ---
> >  security/integrity/ima/ima_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> >  I'm not sure if this patch is appropriate, but even when it's not it
> >  would be interesting to know why it isn't.
> 
> Any input to this one?

Sorry, I'm still thinking about it.  For filesystems that
automatically enable i_version there would be no difference.  For
filesystems that require a mount option to enable i_version, this
changes the behavior.

This is slightly different than not caching the integrity results, in
that the cache is only cleared if someone opens the file rw.

(Jeff Layton posted a patch that replaces the i_version checks with
atime/mtime.)

Mimi 

> Sascha
> 
> >  
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index ac66680689d3..931773049a09 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -123,7 +123,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  
> >  	inode_lock(inode);
> >  	if (atomic_read(&inode->i_writecount) == 1) {
> > -		if ((iint->version != inode->i_version) ||
> > +		if (!inode->i_version || (iint->version != inode->i_version) ||
> >  		    (iint->flags & IMA_NEW_FILE)) {
> >  			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> >  			iint->measured_pcrs = 0;
> > -- 
> > 2.11.0
> > 
> > 
> 

Re: [Linux-ima-devel] [PATCH] ima: Support filesystems without i_version support

[Sascha H. <s....@pe...>]

Mimi,

On Wed, Sep 13, 2017 at 04:15:13PM +0200, Sascha Hauer wrote:
> IMA uses the inode's i_version field to detect changes on an inode.
> This seems to be an optimization for IMA and not strictly necessary.
> Just ignore the i_version field if it is zero and measure the file
> anyway. On filesystems which do not support i_version this may result
> in an unnecessary re-measurement of a file when it has been opened for
> writing without anything actually being written. For filesystems with
> i_version support the behaviour doesn't change.
> 
> Signed-off-by: Sascha Hauer <s....@pe...>
> ---
>  security/integrity/ima/ima_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>  I'm not sure if this patch is appropriate, but even when it's not it
>  would be interesting to know why it isn't.

Any input to this one?

Sascha

>  
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index ac66680689d3..931773049a09 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -123,7 +123,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  
>  	inode_lock(inode);
>  	if (atomic_read(&inode->i_writecount) == 1) {
> -		if ((iint->version != inode->i_version) ||
> +		if (!inode->i_version || (iint->version != inode->i_version) ||
>  		    (iint->flags & IMA_NEW_FILE)) {
>  			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
>  			iint->measured_pcrs = 0;
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Z. <zo...@li...>]

On Mon, 2017-09-18 at 12:13 +0200, Jan Kara wrote:
> On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> > On 17/09/17 17:38, Al Viro wrote:
> > >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> > >>Now, I suspect most (all?) do, but that's a historical artifact rather
> > >>than "design". In particular, the VFS layer used to do the locking for
> > >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> > >>that writes be seen atomically).
> > >>
> > >>But that lock was pushed down into the filesystems, since some
> > >>filesystems really wanted to have parallel writes (particularly for
> > >>direct IO, where that POSIX serialization requirement doesn't exist).
> > >>
> > >>That's all many years ago, though. New filesystems are likely to have
> > >>copied the pattern from old ones, but even then..
> > >>
> > >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> > >>as a lock. You can have the question of *which* inode gets talked
> > >>about when you have things like eoverlayfs etc. Normally it would be
> > >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> > >>same thing in the simple cases), and sometimes it really wouldn't be
> > >>obvious at all..
> > >>
> > >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> > >>one of those things that are "mostly right for the simple cases",
> > >>but...
> > >The thing pretty much common to all of them is that write() might need
> > >to modify permissions (suid removal), which brings ->i_rwsem in one
> > >way or another - notify_change() needs that held...
> > 
> > For GFS2, if we are to hold the inode info constant while it is checked, we
> > would need to take a glock (read lock in this case) across the relevant
> > operations. The glock will be happy under i_rwlock, since we have a lock
> > ordering that takes local locks ahead of cluster locks. I've not dug into
> > this enough to figure out whether the current proposal will allow this to
> > work with GFS2 though. Does IMA cache the results from the
> > ->read_integrity() operation?

Up to now, the hash calculation was stored in the iint structure,
which is then used to extend the TPM, verify the file's integrity
compared to the value stored in the xattr, and included in an audit
message.

A new patch set by Thiago Bauermann will add appended signature
support, re-using the kernel module signature appended method, which
might require re-calculating the file hash based on a different hash
algorithm.

> So I have asked Mimi about clustered filesystems before. And for now the
> answer was that IMA for clustered filesystems is not supported (it will
> return some error since ->integrity_read is NULL). If we would ever want to
> support those it would require larger overhaul of the IMA architecture to
> give filesystem more control over the locking (which is essentially what
> Linus wants).

For performance reasons, IMA is not on a write hook, but detects file
change on the last __fput() opened for write.  At that point, the
cached info is reset.  The file hash is re-calculated and written out
as an xattr.  On the next file access (in policy), the file hash is
re-calculated and stored in the iint.

In terms of remote/clustered/fuse filesystems, we wouldn't be on the
__fput() path.  Support for remote/clustered/fuse filesystems, would
be similar to filesystems that do not support i_version.  Meaning only
the first file access (in policy) would be measured/appraised, but not
subsequent ones.  Even if we could detect file change, we would be
dependent on the remote/clustered/fuse filesystem to inform us of the
change.  What type of integrity guarantees would that provide?

Mimi

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Jan K. <ja...@su...>]

On Mon 18-09-17 10:19:25, Steven Whitehouse wrote:
> On 17/09/17 17:38, Al Viro wrote:
> >On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> >>Now, I suspect most (all?) do, but that's a historical artifact rather
> >>than "design". In particular, the VFS layer used to do the locking for
> >>the filesystems, to guarantee the POSIX requirements (POSIX requires
> >>that writes be seen atomically).
> >>
> >>But that lock was pushed down into the filesystems, since some
> >>filesystems really wanted to have parallel writes (particularly for
> >>direct IO, where that POSIX serialization requirement doesn't exist).
> >>
> >>That's all many years ago, though. New filesystems are likely to have
> >>copied the pattern from old ones, but even then..
> >>
> >>Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> >>as a lock. You can have the question of *which* inode gets talked
> >>about when you have things like eoverlayfs etc. Normally it would be
> >>obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> >>same thing in the simple cases), and sometimes it really wouldn't be
> >>obvious at all..
> >>
> >>So... I'm really not at all convinced that i_rwsem is sensible. It's
> >>one of those things that are "mostly right for the simple cases",
> >>but...
> >The thing pretty much common to all of them is that write() might need
> >to modify permissions (suid removal), which brings ->i_rwsem in one
> >way or another - notify_change() needs that held...
> 
> For GFS2, if we are to hold the inode info constant while it is checked, we
> would need to take a glock (read lock in this case) across the relevant
> operations. The glock will be happy under i_rwlock, since we have a lock
> ordering that takes local locks ahead of cluster locks. I've not dug into
> this enough to figure out whether the current proposal will allow this to
> work with GFS2 though. Does IMA cache the results from the
> ->read_integrity() operation?

So I have asked Mimi about clustered filesystems before. And for now the
answer was that IMA for clustered filesystems is not supported (it will
return some error since ->integrity_read is NULL). If we would ever want to
support those it would require larger overhaul of the IMA architecture to
give filesystem more control over the locking (which is essentially what
Linus wants).

								Honza

-- 
Jan Kara <ja...@su...>
SUSE Labs, CR

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Steven W. <swh...@re...>]

Hi,


On 17/09/17 17:38, Al Viro wrote:
> On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
>> Now, I suspect most (all?) do, but that's a historical artifact rather
>> than "design". In particular, the VFS layer used to do the locking for
>> the filesystems, to guarantee the POSIX requirements (POSIX requires
>> that writes be seen atomically).
>>
>> But that lock was pushed down into the filesystems, since some
>> filesystems really wanted to have parallel writes (particularly for
>> direct IO, where that POSIX serialization requirement doesn't exist).
>>
>> That's all many years ago, though. New filesystems are likely to have
>> copied the pattern from old ones, but even then..
>>
>> Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
>> as a lock. You can have the question of *which* inode gets talked
>> about when you have things like eoverlayfs etc. Normally it would be
>> obvious, but sometimes you'd use "file->f_mapping->host" (which is the
>> same thing in the simple cases), and sometimes it really wouldn't be
>> obvious at all..
>>
>> So... I'm really not at all convinced that i_rwsem is sensible. It's
>> one of those things that are "mostly right for the simple cases",
>> but...
> The thing pretty much common to all of them is that write() might need
> to modify permissions (suid removal), which brings ->i_rwsem in one
> way or another - notify_change() needs that held...

For GFS2, if we are to hold the inode info constant while it is checked, 
we would need to take a glock (read lock in this case) across the 
relevant operations. The glock will be happy under i_rwlock, since we 
have a lock ordering that takes local locks ahead of cluster locks. I've 
not dug into this enough to figure out whether the current proposal will 
allow this to work with GFS2 though. Does IMA cache the results from the 
->read_integrity() operation?

Steve.

[Linux-ima-devel] [SPAM] Custom Made Web Applications For Your Business - Accounting / CRM / Inventory etc (VPB3)

[Helen A. <hel...@te...>]

An HTML attachment was scrubbed...

[SPAM] é‚®å±€é‡è¦æç¤ºè¯·åŠ¡å¿ å°½å¿«é˜ è¯».

[管理员 <eb...@cr...>]

这是一封 HTML 格式的邮件，请以网页方式查看邮件。
-------------- next part --------------
An HTML attachment was scrubbed...

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Al V. <vi...@Ze...>]

On Sun, Sep 17, 2017 at 09:34:01AM -0700, Linus Torvalds wrote:
> Now, I suspect most (all?) do, but that's a historical artifact rather
> than "design". In particular, the VFS layer used to do the locking for
> the filesystems, to guarantee the POSIX requirements (POSIX requires
> that writes be seen atomically).
> 
> But that lock was pushed down into the filesystems, since some
> filesystems really wanted to have parallel writes (particularly for
> direct IO, where that POSIX serialization requirement doesn't exist).
> 
> That's all many years ago, though. New filesystems are likely to have
> copied the pattern from old ones, but even then..
> 
> Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
> as a lock. You can have the question of *which* inode gets talked
> about when you have things like eoverlayfs etc. Normally it would be
> obvious, but sometimes you'd use "file->f_mapping->host" (which is the
> same thing in the simple cases), and sometimes it really wouldn't be
> obvious at all..
> 
> So... I'm really not at all convinced that i_rwsem is sensible. It's
> one of those things that are "mostly right for the simple cases",
> but...

The thing pretty much common to all of them is that write() might need
to modify permissions (suid removal), which brings ->i_rwsem in one
way or another - notify_change() needs that held...

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus T. <tor...@li...>]

On Sun, Sep 17, 2017 at 9:15 AM, Mimi Zohar <zo...@li...> wrote:
>
> Unless I'm missing something, that would only be possible with an IMA
> policy rule that permits direct IO (eg. permit_directio).  Otherwise
> the direct IO is denied.

Note that the "XFS and directio" was only an example.

There is absolutely nothing that says that a  filesystem has to use
i_rwsem for IO serialization at all. Even for the regular write path.

Now, I suspect most (all?) do, but that's a historical artifact rather
than "design". In particular, the VFS layer used to do the locking for
the filesystems, to guarantee the POSIX requirements (POSIX requires
that writes be seen atomically).

But that lock was pushed down into the filesystems, since some
filesystems really wanted to have parallel writes (particularly for
direct IO, where that POSIX serialization requirement doesn't exist).

That's all many years ago, though. New filesystems are likely to have
copied the pattern from old ones, but even then..

Also, it's worth noting that "inode->i_rwlock" isn't even well-defined
as a lock. You can have the question of *which* inode gets talked
about when you have things like eoverlayfs etc. Normally it would be
obvious, but sometimes you'd use "file->f_mapping->host" (which is the
same thing in the simple cases), and sometimes it really wouldn't be
obvious at all..

So... I'm really not at all convinced that i_rwsem is sensible. It's
one of those things that are "mostly right for the simple cases",
but...

              Linus

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Z. <zo...@li...>]

On Sun, 2017-09-17 at 08:28 -0700, Linus Torvalds wrote:
> On Sun, Sep 17, 2017 at 8:17 AM, Christoph Hellwig <hc...@in...> wrote:
> >
> > Only for direct I/O, and IMA and direct I/O don't work together.
> > From ima_collect_measurement:
> >
> >                 if (file->f_flags & O_DIRECT) {
> >                         audit_cause = "failed(directio)";
> >                         result = -EACCES;
> >                         goto out;
> >                 }
> 
> That's not the issue.
> 
> The issue is that somebody else can come in - using direct IO - at the
> same time as the first person is collecting measurements, and thus
> race with the collector.
> 
> So now the measurements are not trustworthy any more.

Unless I'm missing something, that would only be possible with an IMA
policy rule that permits direct IO (eg. permit_directio).  Otherwise
the direct IO is denied.

> > Well, that's exactly the point of the new ->integrity_read routine
> > I proposed and prototype.  The important thing is that it is called
> > with i_rwsem held because code mugh higher in the chain already
> > acquired it, but except for that it's entirely up to the file system.
> 
> .. and *my* point is that it's the wrong lock for actually checking
> integrity (it doesn't actually guarantee exclusion, even though in
> practice it's almost always the case), and so we're adding a nasty
> callback that in 99% of all cases is the same as the normal read, and
> we *could* have just added it with a RWF flag instead.
> 
> Is there some reason why integrity has to use that particular lock
> that is so inconvenient for the filesystems it wants to check?

Originally IMA had its own lock (iint->mutex), prior to IMA-appraisal
being upstreamed.  With a separate lock, the iint->mutex and i_rwsem
would be taken in reverse order in process_measurements() and in the
setxattr, chown, chmod syscalls.

I'm at the airport on my way back home.

Mimi

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Christoph H. <hc...@in...>]

On Sun, Sep 17, 2017 at 08:28:40AM -0700, Linus Torvalds wrote:
> The issue is that somebody else can come in - using direct IO - at the
> same time as the first person is collecting measurements, and thus
> race with the collector.
>
> So now the measurements are not trustworthy any more.

Yes.  And it's always been that way with IMA.

> .. and *my* point is that it's the wrong lock for actually checking
> integrity (it doesn't actually guarantee exclusion, even though in
> practice it's almost always the case), and so we're adding a nasty
> callback that in 99% of all cases is the same as the normal read, and
> we *could* have just added it with a RWF flag instead.
> 
> Is there some reason why integrity has to use that particular lock
> that is so inconvenient for the filesystems it wants to check?

I'll have to defer that to Mimi - I just jumped into this whole mess
to help fixing the deadlocks we saw on XFS and NFS.

Unfortunately the whole security code is a giant mess that doesn't
document assumptions, threat models or gets any sort of verification
of those through automated testing.

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus T. <tor...@li...>]

On Sun, Sep 17, 2017 at 8:17 AM, Christoph Hellwig <hc...@in...> wrote:
>
> Only for direct I/O, and IMA and direct I/O don't work together.
> From ima_collect_measurement:
>
>                 if (file->f_flags & O_DIRECT) {
>                         audit_cause = "failed(directio)";
>                         result = -EACCES;
>                         goto out;
>                 }

That's not the issue.

The issue is that somebody else can come in - using direct IO - at the
same time as the first person is collecting measurements, and thus
race with the collector.

So now the measurements are not trustworthy any more.

> Well, that's exactly the point of the new ->integrity_read routine
> I proposed and prototype.  The important thing is that it is called
> with i_rwsem held because code mugh higher in the chain already
> acquired it, but except for that it's entirely up to the file system.

.. and *my* point is that it's the wrong lock for actually checking
integrity (it doesn't actually guarantee exclusion, even though in
practice it's almost always the case), and so we're adding a nasty
callback that in 99% of all cases is the same as the normal read, and
we *could* have just added it with a RWF flag instead.

Is there some reason why integrity has to use that particular lock
that is so inconvenient for the filesystems it wants to check?

                 Linus

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Christoph H. <hc...@in...>]

On Sat, Sep 16, 2017 at 11:20:47AM -0700, Linus Torvalds wrote:
> Sure, generic_file_write_iter() does take that lock exclusively, but
> not everybody uses generic_file_write_iter() at all for writing.
> 
> For example, xfs still uses that i_rwsem, but for block-aligned writes
> it will only get it shared. And I'm not convinced some other
> filesystem might not end up using some other lock entirely.

Only for direct I/O, and IMA and direct I/O don't work together.
>From ima_collect_measurement:

		if (file->f_flags & O_DIRECT) {
			audit_cause = "failed(directio)";
			result = -EACCES;
			goto out;
		}

(and yes, it should be checking for IOCB_DIRECT to avoid racy
f_flags manipulations, but that's another issue)

> The filesystem can do its own locking, and I'm starting to think that
> it would be better to just pass this "this is an integrity read" down
> to the filesystem, and expect the filesystem to do the locking based
> on that.

Well, that's exactly the point of the new ->integrity_read routine
I proposed and prototype.  The important thing is that it is called
with i_rwsem held because code mugh higher in the chain already
acquired it, but except for that it's entirely up to the file system.

[SPAM] 曾经沧海难为水，除却巫山不是云。____元稹《离思五首·其四 》

[卓若 <jia...@fo...>]

An HTML attachment was scrubbed...

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Z. <zo...@li...>]

On Sat, 2017-09-16 at 11:20 -0700, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 1:25 PM, Mimi Zohar <zo...@li...> wrote:
> >
> > To resolve this locking problem, this patch defines a new
> > ->integrity_read file operation method, which is equivalent to
> > ->read_iter, except that it will not take the i_rwsem lock, but will
> > be called with the i_rwsem held exclusively.
> >
> > Since taking the i_rwsem exclusively is not required for reading the
> > file in order to calculate the file hash, the code only verifies
> > that the lock has been taken.
> 
> Ok, so I'm onboard with the commit message now, but realized that I'm
> not actually convinced that i_rwsem is even meaningful.
> 
> Sure, generic_file_write_iter() does take that lock exclusively, but
> not everybody uses generic_file_write_iter() at all for writing.

> For example, xfs still uses that i_rwsem, but for block-aligned writes
> it will only get it shared. And I'm not convinced some other
> filesystem might not end up using some other lock entirely.
> 
> So I'm basically not entirely convinced that these i_rwsem games make
> any sense at all.
> 
> The filesystem can do its own locking, and I'm starting to think that
> it would be better to just pass this "this is an integrity read" down
> to the filesystem, and expect the filesystem to do the locking based
> on that.

IMA would still need to take the i_rwsem to write the xattr.  Unless
the i_rwsem was taken before calling the integrity_read, calculating
the file hash would be serialized, but would not prevent the file hash
from being calculated multiple times.

(Introducing a new lock would result in the locks being taken in
reverse order for setxattr, chown, chmod syscalls.)

Mimi

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Linus T. <tor...@li...>]

On Fri, Sep 15, 2017 at 1:25 PM, Mimi Zohar <zo...@li...> wrote:
>
> To resolve this locking problem, this patch defines a new
> ->integrity_read file operation method, which is equivalent to
> ->read_iter, except that it will not take the i_rwsem lock, but will
> be called with the i_rwsem held exclusively.
>
> Since taking the i_rwsem exclusively is not required for reading the
> file in order to calculate the file hash, the code only verifies
> that the lock has been taken.

Ok, so I'm onboard with the commit message now, but realized that I'm
not actually convinced that i_rwsem is even meaningful.

Sure, generic_file_write_iter() does take that lock exclusively, but
not everybody uses generic_file_write_iter() at all for writing.

For example, xfs still uses that i_rwsem, but for block-aligned writes
it will only get it shared. And I'm not convinced some other
filesystem might not end up using some other lock entirely.

So I'm basically not entirely convinced that these i_rwsem games make
any sense at all.

The filesystem can do its own locking, and I'm starting to think that
it would be better to just pass this "this is an integrity read" down
to the filesystem, and expect the filesystem to do the locking based
on that.

                 Linus

Re: [Linux-ima-devel] [tpmdd-devel] New ML for TPM and IMA

[Joe P. <jo...@pe...>]

On Fri, 2017-09-15 at 14:47 -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 11:01:51AM -0700, Joe Perches wrote:
[]
> > It's polite to add a CREDITS entry when removing inactive maintainers.
> 
> The file ends:
> 
> "
> # Don't add your name here, unless you really _are_ after Marc
> # alphabetically. Leonard used to be very proud of being the 
> # last entry, and he'll get positively pissed if he can't even
> # be second-to-last.  (and this file really _is_ supposed to be
> # in alphabetic order)
> "
> 
> So where do I should add?-)

http://www.varley.net/Pages/Manhattan.htm

Re: [Linux-ima-devel] [tpmdd-devel] New ML for TPM and IMA

[Jarkko S. <jar...@li...>]

On Fri, Sep 15, 2017 at 11:01:51AM -0700, Joe Perches wrote:
> On Fri, 2017-09-15 at 10:57 -0700, Jarkko Sakkinen wrote:
> > On Fri, Sep 15, 2017 at 10:47:06AM -0700, Peter Huewe wrote:
> > > Am 15. September 2017 10:40:14 GMT-07:00 schrieb Joe Perches <jo...@pe...>:
> > > > As the individual maintainers are different for the two sections,
> > > > I think you need both entries.
> > > 
> > > Try to get a hold of ashley and ask whether she is actively maintaining.
> > > 
> > > While updating Maintainers I vote for removing Marcel Selhorst for tpm.
> > > 
> > > You can keep me for now :)
> > > Peter
> > 
> > During the last bit less than couple of years I've been maintaining
> > TPM and a bit less than four years I've been contributing I've never
> > heard anything of Ashley.
> > 
> > What is the expiration time?
> 
> There is no fixed time at all.
> Generally when the email bounces or when the maintainer resigns.
> (involuntarily or not...)
> 
> It seems a couple years since Ashley signed anything and five years
> since Marcel Selhorst signed anything.
> 
> It's polite to add a CREDITS entry when removing inactive maintainers.

The file ends:

"
# Don't add your name here, unless you really _are_ after Marc
# alphabetically. Leonard used to be very proud of being the 
# last entry, and he'll get positively pissed if he can't even
# be second-to-last.  (and this file really _is_ supposed to be
# in alphabetic order)
"

So where do I should add?-)

/Jarkko

Re: [Linux-ima-devel] [tpmdd-devel] New ML for TPM and IMA

[Peter H. <pet...@gm...>]

Am 15. September 2017 13:07:58 GMT-07:00 schrieb Ken Goldman <kg...@li...>:
>Newbie question:  Do I have to subscribe, or are email addresses being 
>migrated

You have to subscribe yourself.
Due to the new privacy regulations I do not have access to any member information anymore on the old list.


Just send a plaintext  email with the body/text
subscribe linux-integrity
to maj...@vg...

Peter
>
>On 9/15/2017 1:18 PM, Jarkko Sakkinen wrote:
>> Hi
>> 
>> Many people were kicked out from the SourceForge mailing list in the
>> July because SF required a resubscription, including non-human users,
>> such as patchwork.
>> 
>> We decided to create a new mailing list
>lin...@vg...
>> to cover both TPM and IMA since they tend to have cross dependencies.
>> Otherwise, the maintainer hierarchy etc. will stay the same.
>> 
>> It is all documented here:
>> 
>> http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
>> 
>>  From now on use this list for patches and discussion instead of the
>> legacy list.
>
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>tpm...@li...
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data (updated patch description)

[Mimi Z. <zo...@li...>]

From: Christoph Hellwig <hc...@ls...>

Writing extended attributes requires exclusively taking the i_rwsem
lock.  To synchronize the file hash calculation and writing the file
hash as security.ima xattr, IMA-appraisal takes the i_rwsem lock
exclusively before calculating the file hash.  (Once the file hash
is calculated, the result is cached.  Taking the lock exclusively
prevents calculating the file hash multiple times.)

Some filesystems have recently replaced their filesystem dependent
lock with the global i_rwsem to read a file.  As a result, when IMA
attempts to calculate the file hash, reading the file attempts to
take the i_rwsem again.

To resolve this locking problem, this patch defines a new
->integrity_read file operation method, which is equivalent to
->read_iter, except that it will not take the i_rwsem lock, but will
be called with the i_rwsem held exclusively.

Since taking the i_rwsem exclusively is not required for reading the
file in order to calculate the file hash, the code only verifies
that the lock has been taken.

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

Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
the VFS inode instead"
Signed-off-by: Mimi Zohar <zo...@li...>
Reviewed-by: Jan Kara <ja...@su...>
Reviewed-by: Dmitry Kasatkin <dmi...@hu...>

---
Changelog:

- Updated patch description

---
 fs/btrfs/file.c           |  1 +
 fs/efivarfs/file.c        |  1 +
 fs/ext2/file.c            | 17 +++++++++++++++++
 fs/ext4/file.c            | 20 ++++++++++++++++++++
 fs/f2fs/file.c            |  1 +
 fs/jffs2/file.c           |  1 +
 fs/jfs/file.c             |  1 +
 fs/nilfs2/file.c          |  1 +
 fs/ramfs/file-mmu.c       |  1 +
 fs/ramfs/file-nommu.c     |  1 +
 fs/ubifs/file.c           |  1 +
 fs/xfs/xfs_file.c         | 21 +++++++++++++++++++++
 include/linux/fs.h        |  1 +
 mm/shmem.c                |  1 +
 security/integrity/iint.c | 20 ++++++++++++++------
 15 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9e75d8a39aac..2542dc66c85c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3125,6 +3125,7 @@ const struct file_operations btrfs_file_operations = {
 #endif
 	.clone_file_range = btrfs_clone_file_range,
 	.dedupe_file_range = btrfs_dedupe_file_range,
+	.integrity_read = generic_file_read_iter,
 };
 
 void btrfs_auto_defrag_exit(void)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 863f1b100165..17955a92a5b3 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -179,4 +179,5 @@ const struct file_operations efivarfs_file_operations = {
 	.write	= efivarfs_file_write,
 	.llseek	= no_llseek,
 	.unlocked_ioctl = efivarfs_file_ioctl,
+	.integrity_read	= efivarfs_file_read_iter,
 };
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index d34d32bdc944..111069de1973 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+#ifdef CONFIG_FS_DAX
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+	if (IS_DAX(iocb->ki_filp->f_mapping->host))
+		return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 #ifdef CONFIG_FS_DAX
@@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
 	.get_unmapped_area = thp_get_unmapped_area,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= ext2_file_integrity_read_iter,
 };
 
 const struct inode_operations ext2_file_inode_operations = {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 58294c9a7e1d..3ab4105c8578 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -74,6 +74,25 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return generic_file_read_iter(iocb, to);
 }
 
+static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
+					     struct iov_iter *to)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	lockdep_assert_held(&inode->i_rwsem);
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+		return -EIO;
+
+	if (!iov_iter_count(to))
+		return 0; /* skip atime */
+
+#ifdef CONFIG_FS_DAX
+	if (IS_DAX(inode))
+		return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
+#endif
+	return generic_file_read_iter(iocb, to);
+}
+
 /*
  * Called when an inode is released. Note that this is different
  * from ext4_file_open: open gets called at every open, but release
@@ -747,6 +766,7 @@ const struct file_operations ext4_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ext4_fallocate,
+	.integrity_read	= ext4_file_integrity_read_iter,
 };
 
 const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2706130c261b..82ea81da0b2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
 #endif
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index c12476e309c6..5a63034cccf5 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
 	.mmap =		generic_file_readonly_mmap,
 	.fsync =	jffs2_fsync,
 	.splice_read =	generic_file_splice_read,
+	.integrity_read = generic_file_read_iter,
 };
 
 /* jffs2_file_inode_operations */
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 739492c7a3fd..423512a810e4 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= jfs_compat_ioctl,
 #endif
+	.integrity_read	= generic_file_read_iter,
 };
diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index c5fa3dee72fc..55e058ac487f 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
 	/* .release	= nilfs_release_file, */
 	.fsync		= nilfs_sync_file,
 	.splice_read	= generic_file_splice_read,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations nilfs_file_inode_operations = {
diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
index 12af0490322f..4f24d1b589b1 100644
--- a/fs/ramfs/file-mmu.c
+++ b/fs/ramfs/file-mmu.c
@@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.llseek		= generic_file_llseek,
 	.get_unmapped_area	= ramfs_mmu_get_unmapped_area,
+	.integrity_read	= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 2ef7ce75c062..5ee704fa84e0 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
 	.splice_read		= generic_file_splice_read,
 	.splice_write		= iter_file_splice_write,
 	.llseek			= generic_file_llseek,
+	.integrity_read		= generic_file_read_iter,
 };
 
 const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8cad0b19b404..5e52a315e18b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl   = ubifs_compat_ioctl,
 #endif
+	.integrity_read = generic_file_read_iter,
 };
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c4893e226fd8..0a6704b563d6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -292,6 +292,26 @@ xfs_file_read_iter(
 	return ret;
 }
 
+static ssize_t
+xfs_integrity_read(
+	struct kiocb		*iocb,
+	struct iov_iter		*to)
+{
+	struct inode		*inode = file_inode(iocb->ki_filp);
+	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
+
+	lockdep_assert_held(&inode->i_rwsem);
+
+	XFS_STATS_INC(mp, xs_read_calls);
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return -EIO;
+
+	if (IS_DAX(inode))
+		return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
+	return generic_file_read_iter(iocb, to);
+}
+
 /*
  * Zero any on disk space between the current EOF and the new, larger EOF.
  *
@@ -1175,6 +1195,7 @@ const struct file_operations xfs_file_operations = {
 	.fallocate	= xfs_file_fallocate,
 	.clone_file_range = xfs_file_clone_range,
 	.dedupe_file_range = xfs_file_dedupe_range,
+	.integrity_read	= xfs_integrity_read,
 };
 
 const struct file_operations xfs_dir_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e522d25d0836..f6a01d3efce5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1699,6 +1699,7 @@ struct file_operations {
 			u64);
 	ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
 			u64);
+	ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
 } __randomize_layout;
 
 struct inode_operations {
diff --git a/mm/shmem.c b/mm/shmem.c
index b0aa6075d164..805d99011ca4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= shmem_fallocate,
+	.integrity_read	= shmem_file_read_iter,
 #endif
 };
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c84e05866052..c3a07276f745 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -21,6 +21,7 @@
 #include <linux/rbtree.h>
 #include <linux/file.h>
 #include <linux/uaccess.h>
+#include <linux/uio.h>
 #include "integrity.h"
 
 static struct rb_root integrity_iint_tree = RB_ROOT;
@@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  void *addr, unsigned long count)
 {
-	mm_segment_t old_fs;
-	char __user *buf = (char __user *)addr;
+	struct inode *inode = file_inode(file);
+	struct kvec iov = { .iov_base = addr, .iov_len = count };
+	struct kiocb kiocb;
+	struct iov_iter iter;
 	ssize_t ret;
 
+	lockdep_assert_held(&inode->i_rwsem);
+
 	if (!(file->f_mode & FMODE_READ))
 		return -EBADF;
+	if (!file->f_op->integrity_read)
+		return -EBADF;
 
-	old_fs = get_fs();
-	set_fs(get_ds());
-	ret = __vfs_read(file, buf, count, &offset);
-	set_fs(old_fs);
+	init_sync_kiocb(&kiocb, file);
+	kiocb.ki_pos = offset;
+	iov_iter_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
 
+	ret = file->f_op->integrity_read(&kiocb, &iter);
+	BUG_ON(ret == -EIOCBQUEUED);
 	return ret;
 }
 
-- 
2.7.4

Re: [Linux-ima-devel] [tpmdd-devel] New ML for TPM and IMA

[Ken G. <kg...@li...>]

Newbie question:  Do I have to subscribe, or are email addresses being 
migrated?

On 9/15/2017 1:18 PM, Jarkko Sakkinen wrote:
> Hi
> 
> Many people were kicked out from the SourceForge mailing list in the
> July because SF required a resubscription, including non-human users,
> such as patchwork.
> 
> We decided to create a new mailing list lin...@vg...
> to cover both TPM and IMA since they tend to have cross dependencies.
> Otherwise, the maintainer hierarchy etc. will stay the same.
> 
> It is all documented here:
> 
> http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
> 
>  From now on use this list for patches and discussion instead of the
> legacy list.