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

Re: [Linux-ima-devel] [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

[James M. <jm...@na...>]

On Thu, 14 Sep 2017, Christoph Hellwig wrote:

> On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> > So, to be clear, this patch solves the XFS deadlock using a different 
> > approach (to the now reverted integrity_read approach), which Christoph 
> > also says is more correct generally.  Correct?
> 
> No.  It is in addition to the previous patches - the patches were
> correct for the IMA interaction with the I/O path.  It just turns
> out that the function was also reused for reading certificates
> at initialization time, for which that change was incorrect.
> 
> If this series is applied first the integrity_read code is not
> used for that path any more.

Ok.  Sorry I hadn't looked at the code in detail at this stage during the 
conference and wanting to just revert back to something that Linus can 
safely pull before the merge window closes.


-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

[James M. <jm...@na...>]

On Thu, 14 Sep 2017, Christoph Hellwig wrote:

> On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> > So, to be clear, this patch solves the XFS deadlock using a different 
> > approach (to the now reverted integrity_read approach), which Christoph 
> > also says is more correct generally.  Correct?
> 
> No.  It is in addition to the previous patches - the patches were
> correct for the IMA interaction with the I/O path.  It just turns
> out that the function was also reused for reading certificates
> at initialization time, for which that change was incorrect.
> 
> If this series is applied first the integrity_read code is not
> used for that path any more.

Ok, Mimi, please post a complete patchset for this issue against my -next 
branch.


-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

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

On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> So, to be clear, this patch solves the XFS deadlock using a different 
> approach (to the now reverted integrity_read approach), which Christoph 
> also says is more correct generally.  Correct?

No.  It is in addition to the previous patches - the patches were
correct for the IMA interaction with the I/O path.  It just turns
out that the function was also reused for reading certificates
at initialization time, for which that change was incorrect.

If this series is applied first the integrity_read code is not
used for that path any more.

Re: [Linux-ima-devel] [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

[James M. <jm...@na...>]

On Tue, 12 Sep 2017, Mimi Zohar wrote:

> From: Christoph Hellwig <hc...@ls...>
> 
> The CONFIG_IMA_LOAD_X509 and CONFIG_EVM_LOAD_X509 options permit
> loading x509 signed certificates onto the trusted keyrings without
> verifying the x509 certificate file's signature.
> 
> This patch replaces the call to the integrity_read_file() specific
> function with the common kernel_read_file_from_path() function.
> To avoid verifying the file signature, this patch defines
> READING_X509_CERTFICATE.

So, to be clear, this patch solves the XFS deadlock using a different 
approach (to the now reverted integrity_read approach), which Christoph 
also says is more correct generally.  Correct?

What testing has this had?

Should this go in with the rest of the security changes now or wait until 
either -rc or the next merge window?


-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

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

On Thu, Sep 14, 2017 at 02:55:34PM +0530, Nayna Jain wrote:
> 
> 
> On 09/13/2017 06:17 AM, Jarkko Sakkinen wrote:
> > On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
> > > Currently, tpm_msleep() uses delay_msec as the minimum value in
> > > usleep_range. However, that is the maximum time we want to wait.
> > > The function is modified to use the delay_msec as the maximum
> > > value, not the minimum value.
> > > 
> > > After this change, performance on a TPM 1.2 with an 8 byte
> > > burstcount for 1000 extends improved from ~9sec to ~8sec.
> > > 
> > > Signed-off-by: Nayna Jain <na...@li...>
> > > Acked-by: Mimi Zohar <zo...@li...>
> > > ---
> > >   drivers/char/tpm/tpm.h | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index eb2f8818eded..ff5a8b7b80b9 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> > >   static inline void tpm_msleep(unsigned int delay_msec)
> > >   {
> > > -	usleep_range(delay_msec * 1000,
> > > -		     (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
> > > +	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
> > > +		     delay_msec * 1000);
> > >   };
> > >   struct tpm_chip *tpm_chip_find_get(int chip_num);
> > > -- 
> > > 2.13.3
> > > 
> > Doesn't this need a Fixes tag?
> Yeah.. will add.

No need just for that. I'll test this when I'm back in Finland.
It was a question just to check that I'm in the same page  :-)

/Jarkko

Re: [Linux-ima-devel] [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

[Nayna J. <na...@li...>]

On 09/13/2017 06:17 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
>> Currently, tpm_msleep() uses delay_msec as the minimum value in
>> usleep_range. However, that is the maximum time we want to wait.
>> The function is modified to use the delay_msec as the maximum
>> value, not the minimum value.
>>
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~9sec to ~8sec.
>>
>> Signed-off-by: Nayna Jain <na...@li...>
>> Acked-by: Mimi Zohar <zo...@li...>
>> ---
>>   drivers/char/tpm/tpm.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index eb2f8818eded..ff5a8b7b80b9 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>>   
>>   static inline void tpm_msleep(unsigned int delay_msec)
>>   {
>> -	usleep_range(delay_msec * 1000,
>> -		     (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
>> +	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
>> +		     delay_msec * 1000);
>>   };
>>   
>>   struct tpm_chip *tpm_chip_find_get(int chip_num);
>> -- 
>> 2.13.3
>>
> Doesn't this need a Fixes tag?
Yeah.. will add.

    - Nayna
> /Jarkko
>

Re: [Linux-ima-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

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

On Wed, Sep 13, 2017 at 11:39:03AM -0700, Peter Huewe wrote:
> 
> 
> Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <jar...@li...>:
> >On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
> >> The TPM burstcount status indicates the number of bytes that can
> >> be sent to the TPM without causing bus wait states.  Effectively,
> >> it is the number of empty bytes in the command FIFO. Further,
> >> some TPMs have a static burstcount, when the value remains zero
> >> until the entire FIFO is empty.
> >> 
> >> This patch adds an optimization to check for burstcount only once.
> >> And if it is valid, it writes all the bytes at once, permitting
> >> wait states. The performance of a 34 byte extend on a TPM 1.2 with
> >> an 8 byte burstcount improved from 41 msec to 14 msec.
> >> 
> >> This functionality is enabled only by passing module
> >> parameter ignore_burst_count=1. By default, this parameter
> >> is disabled.
> >> 
> >> After this change, performance on a TPM 1.2 with an 8 byte
> >> burstcount for 1000 extends improved from ~41sec to ~14sec.
> >> 
> >> Suggested-by: Ken Goldman <kg...@li...> in
> >> conjunction with the TPM Device Driver work group.
> >> Signed-off-by: Nayna Jain <na...@li...>
> >> Acked-by: Mimi Zohar <zo...@li...>
> >> ---
> >>  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++++
> >>  drivers/char/tpm/tpm_tis_core.c                 | 24
> >+++++++++++++++++++++---
> >>  2 files changed, 29 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> >b/Documentation/admin-guide/kernel-parameters.txt
> >> index 4e303be83df6..3c59bb91e1ee 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1465,6 +1465,14 @@
> >>  			mode generally follows that for the NaN encoding,
> >>  			except where unsupported by hardware.
> >>  
> >> +	ignore_burst_count [TPM_TIS_CORE]
> >> +			tpm_tis_core driver queries for the burstcount before
> >> +			every send call in a loop. However, it causes delay to
> >> +			the send command for TPMs with low burstcount value.
> >> +			Setting this value to 1, will make driver to query for
> >> +			burstcount only once in the loop to improve the
> >> +			performance. By default, its value is set to 0.
> >> +
> >>  	ignore_loglevel	[KNL]
> >>  			Ignore loglevel setting - this will print /all/
> >>  			kernel messages to the console. Useful for debugging.
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c
> >b/drivers/char/tpm/tpm_tis_core.c
> >> index 63bc6c3b949e..6b9bf4c4d434 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -31,6 +31,11 @@
> >>  #include "tpm.h"
> >>  #include "tpm_tis_core.h"
> >>  
> >> +static bool ignore_burst_count = false;
> >> +module_param(ignore_burst_count, bool, 0444);
> >> +MODULE_PARM_DESC(ignore_burst_count,
> >> +		"Ignore burstcount value while writing data");
> >> +
> >>  /* Before we attempt to access the TPM we must see that the valid
> >bit is set.
> >>   * The specification says that this bit is 0 at reset and remains 0
> >until the
> >>   * 'TPM has gone through its self test and initialization and has
> >established
> >> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
> >*chip, u8 *buf, size_t len)
> >>  {
> >>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>  	int rc, status, burstcnt;
> >> +	int sendcnt;
> >>  	size_t count = 0;
> >>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> >>  
> >> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
> >*chip, u8 *buf, size_t len)
> >>  	}
> >>  
> >>  	while (count < len - 1) {
> >> +
> >> +		/*
> >> +		 * Get the initial burstcount to ensure TPM is ready to
> >> +		 * accept data, even when waiting for burstcount is disabled.
> >> +		 */
> >>  		burstcnt = get_burstcount(chip);
> >>  		if (burstcnt < 0) {
> >>  			dev_err(&chip->dev, "Unable to read burstcount\n");
> >>  			rc = burstcnt;
> >>  			goto out_err;
> >>  		}
> >> -		burstcnt = min_t(int, burstcnt, len - count - 1);
> >> +
> >> +		if (ignore_burst_count)
> >> +			sendcnt = len - 1;
> >> +		else
> >> +			sendcnt = min_t(int, burstcnt, len - count - 1);
> >> +
> >>  		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> >> -					 burstcnt, buf + count);
> >> +					 sendcnt, buf + count);
> >>  		if (rc < 0)
> >>  			goto out_err;
> >>  
> >> -		count += burstcnt;
> >> +		count += sendcnt;
> >> +		if (ignore_burst_count)
> >> +			continue;
> >>  
> >>  		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> >>  					&priv->int_queue, false) < 0) {
> >> -- 
> >> 2.13.3
> >> 
> >
> >Makes sense to discuss whether to have the kernel command-line
> >parameter or not before applying this.
> >
> >To fuel the discussion, alternative to this would be:
> >
> >1. Have this always on i.e. no command-line parameter.
> >2. If someone yells, we add the command-line parameter later on.
> >
> According to what I've read in the tcg ddwg group this patch should
> not cause problems on _sane_ tpms.
> 
> I'm not 100%convinced that all tpms are sane all the time, but I think
> we do not want yet another cmdline parameter.
> 
> So if we want to pull it in (and ddwg does not see an issue, so yes)
> it should be on by default, without a kernel parameter.
> 
> If there is a kernel parameter, then it should only be one called
> "failsafe" - which includes the force behavior and maybe the "broken"
> tpm path.
> 
> But I agree with Alex, every additonal code path reduces testing coverage.
> 
> 
> We would be happy to test a "default on" patch.
> 
> Peter
> 
> >/Jarkko

I'm starting to dilate to this direction.

It is hard to believe that any such TPM would be in active use anywhere
assuming that there exist a TPM where this causes issues. This combined
to the assumption that you would run the latest mainline on it makes it
a pretty insignificant scenario.

/Jarkko

Re: [Linux-ima-devel] [PATCH 1/2] vfs: constify path argument to kernel_read_file_from_path

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

On Tue, Sep 12, 2017 at 10:45:33PM -0400, Mimi Zohar wrote:
> This patch constifies the path argument to kernel_read_file_from_path.
> 
> (Extracted from Helwig's patch.)

Feel free to skip this given that it's trivial and you misspelled my
name anyway :)

Re: [Linux-ima-devel] [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

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

Am 13. September 2017 11:52:12 GMT-07:00 schrieb Ken Goldman <kg...@li...>:
>On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
>> 
>> The problem with this approach is that the TPM could totally block
>> the CPU for very long periods of time.
>> 
>> It seems very risky to enable..
>> 
>
>How would you characterize "very long"?
>
>The TPM vendors confirm that they empty the FIFO at internal speeds
>that
>are comparable to the bus speed.  Thus, any stall will be sub-usec.  Is
>that an issue?

If the tpm does behave correctly, this is fine. 
If the tpm hangs for whatever reason, your machine is frozen and you will never figure out why.

That's my concern there.
However ddwg seems fine.

>
>In addition, new TPMs have ever larger FIFO's, making stalls less
>likely
>going forward.
But also reduced the polling loops that introduce the performance penalty ;)

>
>
>------------------------------------------------------------------------------
>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] [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

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

On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
> 
> The problem with this approach is that the TPM could totally block
> the CPU for very long periods of time.
> 
> It seems very risky to enable..
> 

How would you characterize "very long"?

The TPM vendors confirm that they empty the FIFO at internal speeds that
are comparable to the bus speed.  Thus, any stall will be sub-usec.  Is
that an issue?

In addition, new TPMs have ever larger FIFO's, making stalls less likely
going forward.

Re: [Linux-ima-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

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

Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen <jar...@li...>:
>On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states.  Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>> 
>> This patch adds an optimization to check for burstcount only once.
>> And if it is valid, it writes all the bytes at once, permitting
>> wait states. The performance of a 34 byte extend on a TPM 1.2 with
>> an 8 byte burstcount improved from 41 msec to 14 msec.
>> 
>> This functionality is enabled only by passing module
>> parameter ignore_burst_count=1. By default, this parameter
>> is disabled.
>> 
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~41sec to ~14sec.
>> 
>> Suggested-by: Ken Goldman <kg...@li...> in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain <na...@li...>
>> Acked-by: Mimi Zohar <zo...@li...>
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++++
>>  drivers/char/tpm/tpm_tis_core.c                 | 24
>+++++++++++++++++++++---
>>  2 files changed, 29 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>b/Documentation/admin-guide/kernel-parameters.txt
>> index 4e303be83df6..3c59bb91e1ee 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1465,6 +1465,14 @@
>>  			mode generally follows that for the NaN encoding,
>>  			except where unsupported by hardware.
>>  
>> +	ignore_burst_count [TPM_TIS_CORE]
>> +			tpm_tis_core driver queries for the burstcount before
>> +			every send call in a loop. However, it causes delay to
>> +			the send command for TPMs with low burstcount value.
>> +			Setting this value to 1, will make driver to query for
>> +			burstcount only once in the loop to improve the
>> +			performance. By default, its value is set to 0.
>> +
>>  	ignore_loglevel	[KNL]
>>  			Ignore loglevel setting - this will print /all/
>>  			kernel messages to the console. Useful for debugging.
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>> index 63bc6c3b949e..6b9bf4c4d434 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -31,6 +31,11 @@
>>  #include "tpm.h"
>>  #include "tpm_tis_core.h"
>>  
>> +static bool ignore_burst_count = false;
>> +module_param(ignore_burst_count, bool, 0444);
>> +MODULE_PARM_DESC(ignore_burst_count,
>> +		"Ignore burstcount value while writing data");
>> +
>>  /* Before we attempt to access the TPM we must see that the valid
>bit is set.
>>   * The specification says that this bit is 0 at reset and remains 0
>until the
>>   * 'TPM has gone through its self test and initialization and has
>established
>> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  {
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>  	int rc, status, burstcnt;
>> +	int sendcnt;
>>  	size_t count = 0;
>>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>  
>> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  	}
>>  
>>  	while (count < len - 1) {
>> +
>> +		/*
>> +		 * Get the initial burstcount to ensure TPM is ready to
>> +		 * accept data, even when waiting for burstcount is disabled.
>> +		 */
>>  		burstcnt = get_burstcount(chip);
>>  		if (burstcnt < 0) {
>>  			dev_err(&chip->dev, "Unable to read burstcount\n");
>>  			rc = burstcnt;
>>  			goto out_err;
>>  		}
>> -		burstcnt = min_t(int, burstcnt, len - count - 1);
>> +
>> +		if (ignore_burst_count)
>> +			sendcnt = len - 1;
>> +		else
>> +			sendcnt = min_t(int, burstcnt, len - count - 1);
>> +
>>  		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> -					 burstcnt, buf + count);
>> +					 sendcnt, buf + count);
>>  		if (rc < 0)
>>  			goto out_err;
>>  
>> -		count += burstcnt;
>> +		count += sendcnt;
>> +		if (ignore_burst_count)
>> +			continue;
>>  
>>  		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>>  					&priv->int_queue, false) < 0) {
>> -- 
>> 2.13.3
>> 
>
>Makes sense to discuss whether to have the kernel command-line
>parameter or not before applying this.
>
>To fuel the discussion, alternative to this would be:
>
>1. Have this always on i.e. no command-line parameter.
>2. If someone yells, we add the command-line parameter later on.
>
According to what I've read in the tcg ddwg group this patch should not cause problems on _sane_ tpms.

I'm not 100%convinced that all tpms are sane all the time, but I think we do not want yet another cmdline parameter.

So if we want to pull it in (and ddwg does not see an issue, so yes) it should be on by default, without a kernel parameter.

If there is a kernel parameter, then it should only be one called "failsafe" - which includes the force behavior and maybe the "broken" tpm path.

But I agree with Alex, every additonal code path reduces testing coverage.


We would be happy to test a "default on" patch.

Peter

>/Jarkko

-- 
Sent from my mobile

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

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

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

[Linux-ima-devel] [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

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

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

The CONFIG_IMA_LOAD_X509 and CONFIG_EVM_LOAD_X509 options permit
loading x509 signed certificates onto the trusted keyrings without
verifying the x509 certificate file's signature.

This patch replaces the call to the integrity_read_file() specific
function with the common kernel_read_file_from_path() function.
To avoid verifying the file signature, this patch defines
READING_X509_CERTFICATE.

Signed-off-by: Christoph Hellwig <hc...@ls...>
Signed-off-by: Mimi Zohar <zo...@li...>

---
Changelog:
- rewrote patch description
- fixed parameters to kernel_read_file_from_path() and key_create_or_update()
- defined READING_X509_CERTIFICATE, a new __kernel_read_file enumeration
- removed constify change
---
 include/linux/fs.h                |  1 +
 security/integrity/digsig.c       | 14 +++++++----
 security/integrity/iint.c         | 49 ---------------------------------------
 security/integrity/ima/ima_main.c |  4 ++++
 security/integrity/integrity.h    |  2 --
 5 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index d783cc8340de..e522d25d0836 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2751,6 +2751,7 @@ extern int do_pipe_flags(int *, int);
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
 	id(POLICY, security-policy)		\
+	id(X509_CERTIFICATE, x509-certificate)	\
 	id(MAX_ID, )
 
 #define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..6f9e4ce568cd 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,21 +112,25 @@ int __init integrity_init_keyring(const unsigned int id)
 int __init integrity_load_x509(const unsigned int id, const char *path)
 {
 	key_ref_t key;
-	char *data;
+	void *data;
+	loff_t size;
 	int rc;
 
 	if (!keyring[id])
 		return -EINVAL;
 
-	rc = integrity_read_file(path, &data);
-	if (rc < 0)
+	rc = kernel_read_file_from_path(path, &data, &size, 0,
+					READING_X509_CERTIFICATE);
+	if (rc < 0) {
+		pr_err("Unable to open file: %s (%d)", path, rc);
 		return rc;
+	}
 
 	key = key_create_or_update(make_key_ref(keyring[id], 1),
 				   "asymmetric",
 				   NULL,
 				   data,
-				   rc,
+				   size,
 				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
 				    KEY_USR_VIEW | KEY_USR_READ),
 				   KEY_ALLOC_NOT_IN_QUOTA);
@@ -139,6 +143,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
 			  key_ref_to_ptr(key)->description, path);
 		key_ref_put(key);
 	}
-	kfree(data);
+	vfree(data);
 	return 0;
 }
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..c84e05866052 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 }
 
 /*
- * integrity_read_file - read entire file content into the buffer
- *
- * This is function opens a file, allocates the buffer of required
- * size, read entire file content to the buffer and closes the file
- *
- * It is used only by init code.
- *
- */
-int __init integrity_read_file(const char *path, char **data)
-{
-	struct file *file;
-	loff_t size;
-	char *buf;
-	int rc = -EINVAL;
-
-	if (!path || !*path)
-		return -EINVAL;
-
-	file = filp_open(path, O_RDONLY, 0);
-	if (IS_ERR(file)) {
-		rc = PTR_ERR(file);
-		pr_err("Unable to open file: %s (%d)", path, rc);
-		return rc;
-	}
-
-	size = i_size_read(file_inode(file));
-	if (size <= 0)
-		goto out;
-
-	buf = kmalloc(size, GFP_KERNEL);
-	if (!buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	rc = integrity_kernel_read(file, 0, buf, size);
-	if (rc == size) {
-		*data = buf;
-	} else {
-		kfree(buf);
-		if (rc >= 0)
-			rc = -EIO;
-	}
-out:
-	fput(file);
-	return rc;
-}
-
-/*
  * integrity_load_keys - load integrity keys hook
  *
  * Hooks is called from init/main.c:kernel_init_freeable()
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b00186914df8..72bd2b666a31 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -413,6 +413,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
 		return 0;
 
+	/* permit signed certs */
+	if (!file && read_id == READING_X509_CERTIFICATE)
+		return 0;
+
 	if (!file || !buf || size == 0) { /* should never happen */
 		if (ima_appraise & IMA_APPRAISE_ENFORCE)
 			return -EACCES;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..e1bf040fb110 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 int integrity_kernel_read(struct file *file, loff_t offset,
 			  void *addr, unsigned long count);
 
-int __init integrity_read_file(const char *path, char **data);
-
 #define INTEGRITY_KEYRING_EVM		0
 #define INTEGRITY_KEYRING_IMA		1
 #define INTEGRITY_KEYRING_MODULE	2
-- 
2.7.4

[Linux-ima-devel] [PATCH 1/2] vfs: constify path argument to kernel_read_file_from_path

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

This patch constifies the path argument to kernel_read_file_from_path.

(Extracted from Helwig's patch.)
Signed-off-by: Mimi Zohar <zo...@li...>
---
 fs/exec.c                  | 2 +-
 include/linux/fs.h         | 2 +-
 sound/oss/sound_firmware.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 62175cbcc801..54a4847649cc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
 
-int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
 			       loff_t max_size, enum kernel_read_file_id id)
 {
 	struct file *file;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fdec9b763b54..d783cc8340de 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2775,7 +2775,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 extern int kernel_read(struct file *, loff_t, char *, unsigned long);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
 			    enum kernel_read_file_id);
-extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
+extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
 				      enum kernel_read_file_id);
 extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
 				    enum kernel_read_file_id);
diff --git a/sound/oss/sound_firmware.h b/sound/oss/sound_firmware.h
index da4c67e005ed..2be465277ba0 100644
--- a/sound/oss/sound_firmware.h
+++ b/sound/oss/sound_firmware.h
@@ -21,7 +21,7 @@ static inline int mod_firmware_load(const char *fn, char **fp)
 	loff_t size;
 	int err;
 
-	err = kernel_read_file_from_path((char *)fn, (void **)fp, &size,
+	err = kernel_read_file_from_path(fn, (void **)fp, &size,
 					 131072, READING_FIRMWARE);
 	if (err < 0)
 		return 0;
-- 
2.7.4

Re: [Linux-ima-devel] [PATCH v2 3/4] tpm: reduce tpm_msleep() time in get_burstcount()

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

On Wed, Sep 06, 2017 at 08:56:38AM -0400, Nayna Jain wrote:
> Currently, get_burstcount() function sleeps for 5msec in a loop
> before retrying for next query to burstcount. However, if it takes
> lesser time for TPM to return, this 5 msec delay is longer
> than necessary.
> 
> This patch replaces the tpm_msleep time from 5msec to 1msec.
> 
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~10sec to ~9sec.
> 
> Signed-off-by: Nayna Jain <na...@li...>
> Acked-by: Mimi Zohar <zo...@li...>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index d1eab29cb447..d710bbc4608b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -169,7 +169,7 @@ static int get_burstcount(struct tpm_chip *chip)
>  		burstcnt = (value >> 8) & 0xFFFF;
>  		if (burstcnt)
>  			return burstcnt;
> -		tpm_msleep(TPM_TIMEOUT);
> +		tpm_msleep(1);
>  	} while (time_before(jiffies, stop));
>  	return -EBUSY;
>  }
> -- 
> 2.13.3

How did you pick 1 ms delay? Should there be a constant defining it?

/Jarkko

Re: [Linux-ima-devel] [PATCH v2 2/4] tpm: define __wait_for_tpm_stat to specify variable polling sleep time

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

On Wed, Sep 06, 2017 at 08:56:37AM -0400, Nayna Jain wrote:
> The existing wait_for_tpm_stat() checks the chip status before
> sleeping for 5 msec in a polling loop.  For some functions although
> the status isn't ready immediately, the status returns extremely
> quickly. Waiting for 5 msec causes an unnecessary delay. An
> example is the send() call in the tpms_tis driver.
> 
> This patch defines __wait_for_tpm_stat(), allowing the caller
> to specify the polling sleep timeout value within the loop.
> The existing wait_for_tpm_stat() becomes a wrapper for this
> function.
> 
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~14sec to ~10sec.
> 
> Signed-off-by: Nayna Jain <na...@li...>
> Acked-by: Mimi Zohar <zo...@li...>

Please get rid of wait_for_tpm_stat() rather than further making it more
complex. It's hardware specific stuff. This function should not exist in
tpm-interface.c.

/Jarkko

> ---
>  drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
>  drivers/char/tpm/tpm.h           |  3 +++
>  drivers/char/tpm/tpm_tis_core.c  | 11 ++++++-----
>  3 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1d6729be4cd6..b23d006243b7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1050,8 +1050,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>  	return false;
>  }
>  
> -int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> -		      wait_queue_head_t *queue, bool check_cancel)
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> +		      unsigned int poll_sleep, wait_queue_head_t *queue,
> +		      bool check_cancel)
>  {
>  	unsigned long stop;
>  	long rc;
> @@ -1085,7 +1086,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>  		}
>  	} else {
>  		do {
> -			tpm_msleep(TPM_TIMEOUT);
> +			tpm_msleep(poll_sleep);
>  			status = chip->ops->status(chip);
>  			if ((status & mask) == mask)
>  				return 0;
> @@ -1093,6 +1094,14 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>  	}
>  	return -ETIME;
>  }
> +EXPORT_SYMBOL_GPL(__wait_for_tpm_stat);
> +
> +int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> +		      wait_queue_head_t *queue, bool check_cancel)
> +{
> +	return __wait_for_tpm_stat(chip, mask, timeout, TPM_TIMEOUT,
> +			queue, check_cancel);
> +}
>  EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
>  
>  #define TPM_ORD_SAVESTATE 152
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 2d5466a72e40..eb2f8818eded 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -525,6 +525,9 @@ int tpm_do_selftest(struct tpm_chip *chip);
>  unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>  int tpm_pm_suspend(struct device *dev);
>  int tpm_pm_resume(struct device *dev);
> +int __wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> +		unsigned long timeout, unsigned int poll_sleep,
> +		wait_queue_head_t *queue, bool check_cancel);
>  int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>  		      wait_queue_head_t *queue, bool check_cancel);
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 6b9bf4c4d434..d1eab29cb447 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -268,8 +268,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	status = tpm_tis_status(chip);
>  	if ((status & TPM_STS_COMMAND_READY) == 0) {
>  		tpm_tis_ready(chip);
> -		if (wait_for_tpm_stat
> -		    (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
> +		if (__wait_for_tpm_stat
> +		    (chip, TPM_STS_COMMAND_READY, chip->timeout_b, 1,
>  		     &priv->int_queue, false) < 0) {
>  			rc = -ETIME;
>  			goto out_err;
> @@ -303,7 +303,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  		if (ignore_burst_count)
>  			continue;
>  
> -		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> +		if (__wait_for_tpm_stat(chip, TPM_STS_VALID,
> +					chip->timeout_c, 1,
>  					&priv->int_queue, false) < 0) {
>  			rc = -ETIME;
>  			goto out_err;
> @@ -320,8 +321,8 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	if (rc < 0)
>  		goto out_err;
>  
> -	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> -				&priv->int_queue, false) < 0) {
> +	if (__wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> +				1, &priv->int_queue, false) < 0) {
>  		rc = -ETIME;
>  		goto out_err;
>  	}
> -- 
> 2.13.3
> 

Re: [Linux-ima-devel] [PATCH v2 4/4] tpm: use tpm_msleep() value as max delay

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

On Wed, Sep 06, 2017 at 08:56:39AM -0400, Nayna Jain wrote:
> Currently, tpm_msleep() uses delay_msec as the minimum value in
> usleep_range. However, that is the maximum time we want to wait.
> The function is modified to use the delay_msec as the maximum
> value, not the minimum value.
> 
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~9sec to ~8sec.
> 
> Signed-off-by: Nayna Jain <na...@li...>
> Acked-by: Mimi Zohar <zo...@li...>
> ---
>  drivers/char/tpm/tpm.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index eb2f8818eded..ff5a8b7b80b9 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -533,8 +533,8 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>  
>  static inline void tpm_msleep(unsigned int delay_msec)
>  {
> -	usleep_range(delay_msec * 1000,
> -		     (delay_msec * 1000) + TPM_TIMEOUT_RANGE_US);
> +	usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
> +		     delay_msec * 1000);
>  };
>  
>  struct tpm_chip *tpm_chip_find_get(int chip_num);
> -- 
> 2.13.3
> 

Doesn't this need a Fixes tag?

/Jarkko

Re: [Linux-ima-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

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

On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
> The TPM burstcount status indicates the number of bytes that can
> be sent to the TPM without causing bus wait states.  Effectively,
> it is the number of empty bytes in the command FIFO. Further,
> some TPMs have a static burstcount, when the value remains zero
> until the entire FIFO is empty.
> 
> This patch adds an optimization to check for burstcount only once.
> And if it is valid, it writes all the bytes at once, permitting
> wait states. The performance of a 34 byte extend on a TPM 1.2 with
> an 8 byte burstcount improved from 41 msec to 14 msec.
> 
> This functionality is enabled only by passing module
> parameter ignore_burst_count=1. By default, this parameter
> is disabled.
> 
> After this change, performance on a TPM 1.2 with an 8 byte
> burstcount for 1000 extends improved from ~41sec to ~14sec.
> 
> Suggested-by: Ken Goldman <kg...@li...> in
> conjunction with the TPM Device Driver work group.
> Signed-off-by: Nayna Jain <na...@li...>
> Acked-by: Mimi Zohar <zo...@li...>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  8 ++++++++
>  drivers/char/tpm/tpm_tis_core.c                 | 24 +++++++++++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4e303be83df6..3c59bb91e1ee 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1465,6 +1465,14 @@
>  			mode generally follows that for the NaN encoding,
>  			except where unsupported by hardware.
>  
> +	ignore_burst_count [TPM_TIS_CORE]
> +			tpm_tis_core driver queries for the burstcount before
> +			every send call in a loop. However, it causes delay to
> +			the send command for TPMs with low burstcount value.
> +			Setting this value to 1, will make driver to query for
> +			burstcount only once in the loop to improve the
> +			performance. By default, its value is set to 0.
> +
>  	ignore_loglevel	[KNL]
>  			Ignore loglevel setting - this will print /all/
>  			kernel messages to the console. Useful for debugging.
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 63bc6c3b949e..6b9bf4c4d434 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -31,6 +31,11 @@
>  #include "tpm.h"
>  #include "tpm_tis_core.h"
>  
> +static bool ignore_burst_count = false;
> +module_param(ignore_burst_count, bool, 0444);
> +MODULE_PARM_DESC(ignore_burst_count,
> +		"Ignore burstcount value while writing data");
> +
>  /* Before we attempt to access the TPM we must see that the valid bit is set.
>   * The specification says that this bit is 0 at reset and remains 0 until the
>   * 'TPM has gone through its self test and initialization and has established
> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int rc, status, burstcnt;
> +	int sendcnt;
>  	size_t count = 0;
>  	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>  
> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  	}
>  
>  	while (count < len - 1) {
> +
> +		/*
> +		 * Get the initial burstcount to ensure TPM is ready to
> +		 * accept data, even when waiting for burstcount is disabled.
> +		 */
>  		burstcnt = get_burstcount(chip);
>  		if (burstcnt < 0) {
>  			dev_err(&chip->dev, "Unable to read burstcount\n");
>  			rc = burstcnt;
>  			goto out_err;
>  		}
> -		burstcnt = min_t(int, burstcnt, len - count - 1);
> +
> +		if (ignore_burst_count)
> +			sendcnt = len - 1;
> +		else
> +			sendcnt = min_t(int, burstcnt, len - count - 1);
> +
>  		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> -					 burstcnt, buf + count);
> +					 sendcnt, buf + count);
>  		if (rc < 0)
>  			goto out_err;
>  
> -		count += burstcnt;
> +		count += sendcnt;
> +		if (ignore_burst_count)
> +			continue;
>  
>  		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>  					&priv->int_queue, false) < 0) {
> -- 
> 2.13.3
> 

Makes sense to discuss whether to have the kernel command-line
parameter or not before applying this.

To fuel the discussion, alternative to this would be:

1. Have this always on i.e. no command-line parameter.
2. If someone yells, we add the command-line parameter later on.

/Jarkko

Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

[Richard W. <ri...@no...>]

Sascha,

Am Dienstag, 12. September 2017, 16:23:18 CEST schrieb Sascha Hauer:
> > So, for the IMA use-case we don't even have to persist i_version.
> > That would be cool.
> 
> Yes, that's what earlier versions of this patch did, nacked by Christoph
> 
> Hellwig with the words:
> > Maybe IMA doesn't care, but if you set MS_I_VERSION the fs does give
> > a guarantee.  Sp NAK on this patch as-is.
> 
> (see https://lkml.org/lkml/2017/4/12/61)
> 
> Reading this sentence again it may be a possibility to just increase the
> i_version field without setting the MS_I_VERSION flag.

Yes.

> > I need to read what other filesystems do, it is still not completely clear
> > to me what the expected i_version semantics are. Satisfying IMA seems to
> > be easy but we need to be very sure to not break other futuer i_version
> > users...
> Sure. I am also not sure whether I implemented it correctly since it's
> implementation defined by some filesystem drivers which I am afraid are
> not even consistent.

As usual, let's try to keep at least UBIFS kind of sane. ;-)

Thanks,
//richard

Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

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

On Tue, Sep 12, 2017 at 03:57:57PM +0200, Richard Weinberger wrote:
> Sascha,
> 
> Am Dienstag, 12. September 2017, 15:46:16 CEST schrieb Sascha Hauer:
> > On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote:
> > > Sascha,
> > > 
> > > Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer:
> > > > This adds i_version support to UBIFS. The inodes i_version is used by
> > > > IMA to detect changes to an inode and thus necessary to support IMA on
> > > > UBIFS. The i_version is stored in the previously unused space in the
> > > > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally
> > > > enabled in UBIFS as I saw no reason to make it optional.
> > > 
> > > But we need a new UBIFS feature flag to indicate that this filesystem has
> > > valid i_version fields.
> > 
> > I assume you mean a new UBIFS_FLG_*, right?
> 
> Yes.
>  
> > Who should set this flag? The Kernel once the filesystem has been
> > mounted with iversion support enabled? This would mean we indeed need a
> > iversion mount flag to give the user a chance to continue without
> > iversion support and keep the filesystem compatible with older kernels.
> 
> mkfs.ubifs or the kernel for a new default filesystem.
> Isn't mounting a i_version enabled filesystem without i_version support
> a bad idea since the version counters will be out of sync?

We should probably prevent an old kernel from mounting a UBIFS with
i_version support enabled since that would reset the counters for files
we write to back to 0.

When we mount a UBIFS containing i_version without i_version support the
kernel simply won't increase the i_version numbers on a file write. I
don't think this is a problem. I bet this is the behaviour with ext4
aswell (though I haven't tested it).

> 
> > > > Signed-off-by: Sascha Hauer <s....@pe...>
> > > > ---
> > > > 
> > > >  fs/ubifs/dir.c         | 30 +++++++++++++++++++-----------
> > > >  fs/ubifs/file.c        |  5 +++++
> > > >  fs/ubifs/journal.c     |  3 ++-
> > > >  fs/ubifs/super.c       |  2 ++
> > > >  fs/ubifs/ubifs-media.h |  3 ++-
> > > >  5 files changed, 30 insertions(+), 13 deletions(-)
> > > > 
> > > > I did this patch exclusively to support IMA on UBIFS. IMA uses the
> > > > inode's
> > > > i_version field to detect changes on inodes. A proper i_version support
> > > > needs to make the i_version persistent on disk, although IMA itself
> > > > doesn't
> > > > need a persistent i_version. Last time an earlier version of this patch
> > > > 
> > > > was sent by Oleksij Rempel Richard said:
> > > > > What about making i_version persistent?
> > > > > We still have some empty fields in UBIFS' inode data structure.
> > > > > But first we have to be very sure that we need it.
> > > > 
> > > > This patch exactly implements this suggestion, leaving the question if
> > > > we
> > > > really need it. I added the IMA maintainers to Cc in the hope that Mimi
> > > > or
> > > > Dmitry can give a good reason why there's no alternative to i_version
> > > > for
> > > > IMA.
> > > 
> > > Yes, it would be good to know more about the user, IMA. Does IMA store the
> > > version somewhere?
> > 
> > No. IMA solely uses i_version to detect if an inode has been changed since
> > the last time it has seen this inode.
> > IMA measures all files it hasn't seen before initially and stores the
> > i_version in a struct integrity_iint_cache *. When an inode is written to
> > next time IMA checks if the cached i_version still matches the inode's
> > i_version and if it doesn't, it re-measures the inode. All this is purely
> > runtime.
> > 
> > > Are there requirements on ordering? i.e. What if UBIFS faces a power-cut
> > > and the UBIFS i_version is behind IMA's version.
> > 
> > Since IMA doesn't store the i_version anywhere this won't happen.
> > 
> > > Maybe we have to teach UBIFS to update an inode less lazy that it
> > > currently
> > > does...
> > 
> > No, I don't think so.
> 
> So, for the IMA use-case we don't even have to persist i_version.
> That would be cool.

Yes, that's what earlier versions of this patch did, nacked by Christoph
Hellwig with the words:

> Maybe IMA doesn't care, but if you set MS_I_VERSION the fs does give
> a guarantee.  Sp NAK on this patch as-is.

(see https://lkml.org/lkml/2017/4/12/61)

Reading this sentence again it may be a possibility to just increase the
i_version field without setting the MS_I_VERSION flag.

> 
> I need to read what other filesystems do, it is still not completely clear to 
> me what the expected i_version semantics are. Satisfying IMA seems to be easy
> but we need to be very sure to not break other futuer i_version users...

Sure. I am also not sure whether I implemented it correctly since it's
implementation defined by some filesystem drivers which I am afraid are
not even consistent.

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 |

Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

[Richard W. <ri...@no...>]

Sascha,

Am Dienstag, 12. September 2017, 15:46:16 CEST schrieb Sascha Hauer:
> On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote:
> > Sascha,
> > 
> > Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer:
> > > This adds i_version support to UBIFS. The inodes i_version is used by
> > > IMA to detect changes to an inode and thus necessary to support IMA on
> > > UBIFS. The i_version is stored in the previously unused space in the
> > > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally
> > > enabled in UBIFS as I saw no reason to make it optional.
> > 
> > But we need a new UBIFS feature flag to indicate that this filesystem has
> > valid i_version fields.
> 
> I assume you mean a new UBIFS_FLG_*, right?

Yes.
 
> Who should set this flag? The Kernel once the filesystem has been
> mounted with iversion support enabled? This would mean we indeed need a
> iversion mount flag to give the user a chance to continue without
> iversion support and keep the filesystem compatible with older kernels.

mkfs.ubifs or the kernel for a new default filesystem.
Isn't mounting a i_version enabled filesystem without i_version support
a bad idea since the version counters will be out of sync?

> > > Signed-off-by: Sascha Hauer <s....@pe...>
> > > ---
> > > 
> > >  fs/ubifs/dir.c         | 30 +++++++++++++++++++-----------
> > >  fs/ubifs/file.c        |  5 +++++
> > >  fs/ubifs/journal.c     |  3 ++-
> > >  fs/ubifs/super.c       |  2 ++
> > >  fs/ubifs/ubifs-media.h |  3 ++-
> > >  5 files changed, 30 insertions(+), 13 deletions(-)
> > > 
> > > I did this patch exclusively to support IMA on UBIFS. IMA uses the
> > > inode's
> > > i_version field to detect changes on inodes. A proper i_version support
> > > needs to make the i_version persistent on disk, although IMA itself
> > > doesn't
> > > need a persistent i_version. Last time an earlier version of this patch
> > > 
> > > was sent by Oleksij Rempel Richard said:
> > > > What about making i_version persistent?
> > > > We still have some empty fields in UBIFS' inode data structure.
> > > > But first we have to be very sure that we need it.
> > > 
> > > This patch exactly implements this suggestion, leaving the question if
> > > we
> > > really need it. I added the IMA maintainers to Cc in the hope that Mimi
> > > or
> > > Dmitry can give a good reason why there's no alternative to i_version
> > > for
> > > IMA.
> > 
> > Yes, it would be good to know more about the user, IMA. Does IMA store the
> > version somewhere?
> 
> No. IMA solely uses i_version to detect if an inode has been changed since
> the last time it has seen this inode.
> IMA measures all files it hasn't seen before initially and stores the
> i_version in a struct integrity_iint_cache *. When an inode is written to
> next time IMA checks if the cached i_version still matches the inode's
> i_version and if it doesn't, it re-measures the inode. All this is purely
> runtime.
> 
> > Are there requirements on ordering? i.e. What if UBIFS faces a power-cut
> > and the UBIFS i_version is behind IMA's version.
> 
> Since IMA doesn't store the i_version anywhere this won't happen.
> 
> > Maybe we have to teach UBIFS to update an inode less lazy that it
> > currently
> > does...
> 
> No, I don't think so.

So, for the IMA use-case we don't even have to persist i_version.
That would be cool.

I need to read what other filesystems do, it is still not completely clear to 
me what the expected i_version semantics are. Satisfying IMA seems to be easy
but we need to be very sure to not break other futuer i_version users...

Thanks,
//richard

Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

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

On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote:
> Sascha,
> 
> Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer:
> > This adds i_version support to UBIFS. The inodes i_version is used by
> > IMA to detect changes to an inode and thus necessary to support IMA on
> > UBIFS. The i_version is stored in the previously unused space in the
> > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally
> > enabled in UBIFS as I saw no reason to make it optional.
> 
> But we need a new UBIFS feature flag to indicate that this filesystem has 
> valid i_version fields.

I assume you mean a new UBIFS_FLG_*, right?

Who should set this flag? The Kernel once the filesystem has been
mounted with iversion support enabled? This would mean we indeed need a
iversion mount flag to give the user a chance to continue without
iversion support and keep the filesystem compatible with older kernels.

> 
> > Signed-off-by: Sascha Hauer <s....@pe...>
> > ---
> >  fs/ubifs/dir.c         | 30 +++++++++++++++++++-----------
> >  fs/ubifs/file.c        |  5 +++++
> >  fs/ubifs/journal.c     |  3 ++-
> >  fs/ubifs/super.c       |  2 ++
> >  fs/ubifs/ubifs-media.h |  3 ++-
> >  5 files changed, 30 insertions(+), 13 deletions(-)
> > 
> > I did this patch exclusively to support IMA on UBIFS. IMA uses the inode's
> > i_version field to detect changes on inodes. A proper i_version support
> > needs to make the i_version persistent on disk, although IMA itself doesn't
> > need a persistent i_version. Last time an earlier version of this patch
> > 
> > was sent by Oleksij Rempel Richard said:
> > > What about making i_version persistent?
> > > We still have some empty fields in UBIFS' inode data structure.
> > > But first we have to be very sure that we need it.
> > 
> > This patch exactly implements this suggestion, leaving the question if we
> > really need it. I added the IMA maintainers to Cc in the hope that Mimi or
> > Dmitry can give a good reason why there's no alternative to i_version for
> > IMA.
> 
> Yes, it would be good to know more about the user, IMA. Does IMA store the 
> version somewhere?

No. IMA solely uses i_version to detect if an inode has been changed since
the last time it has seen this inode.
IMA measures all files it hasn't seen before initially and stores the i_version
in a struct integrity_iint_cache *. When an inode is written to next time IMA
checks if the cached i_version still matches the inode's i_version and if it
doesn't, it re-measures the inode. All this is purely runtime.

> Are there requirements on ordering? i.e. What if UBIFS faces a power-cut
> and the UBIFS i_version is behind IMA's version.

Since IMA doesn't store the i_version anywhere this won't happen.

> Maybe we have to teach UBIFS to update an inode less lazy that it currently 
> does...

No, I don't think so.

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 |

Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

[Richard W. <ri...@no...>]

Sascha,

Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer:
> This adds i_version support to UBIFS. The inodes i_version is used by
> IMA to detect changes to an inode and thus necessary to support IMA on
> UBIFS. The i_version is stored in the previously unused space in the
> UBIFS inode struct. Unlike in ext4 i_version support is unconditionally
> enabled in UBIFS as I saw no reason to make it optional.

But we need a new UBIFS feature flag to indicate that this filesystem has 
valid i_version fields.

> Signed-off-by: Sascha Hauer <s....@pe...>
> ---
>  fs/ubifs/dir.c         | 30 +++++++++++++++++++-----------
>  fs/ubifs/file.c        |  5 +++++
>  fs/ubifs/journal.c     |  3 ++-
>  fs/ubifs/super.c       |  2 ++
>  fs/ubifs/ubifs-media.h |  3 ++-
>  5 files changed, 30 insertions(+), 13 deletions(-)
> 
> I did this patch exclusively to support IMA on UBIFS. IMA uses the inode's
> i_version field to detect changes on inodes. A proper i_version support
> needs to make the i_version persistent on disk, although IMA itself doesn't
> need a persistent i_version. Last time an earlier version of this patch
> 
> was sent by Oleksij Rempel Richard said:
> > What about making i_version persistent?
> > We still have some empty fields in UBIFS' inode data structure.
> > But first we have to be very sure that we need it.
> 
> This patch exactly implements this suggestion, leaving the question if we
> really need it. I added the IMA maintainers to Cc in the hope that Mimi or
> Dmitry can give a good reason why there's no alternative to i_version for
> IMA.

Yes, it would be good to know more about the user, IMA. Does IMA store the 
version somewhere?
Are there requirements on ordering? i.e. What if UBIFS faces a power-cut
and the UBIFS i_version is behind IMA's version.
Maybe we have to teach UBIFS to update an inode less lazy that it currently 
does...

Thanks,
//richard

[Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

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

This adds i_version support to UBIFS. The inodes i_version is used by
IMA to detect changes to an inode and thus necessary to support IMA on
UBIFS. The i_version is stored in the previously unused space in the
UBIFS inode struct. Unlike in ext4 i_version support is unconditionally
enabled in UBIFS as I saw no reason to make it optional.

Signed-off-by: Sascha Hauer <s....@pe...>
---
 fs/ubifs/dir.c         | 30 +++++++++++++++++++-----------
 fs/ubifs/file.c        |  5 +++++
 fs/ubifs/journal.c     |  3 ++-
 fs/ubifs/super.c       |  2 ++
 fs/ubifs/ubifs-media.h |  3 ++-
 5 files changed, 30 insertions(+), 13 deletions(-)

I did this patch exclusively to support IMA on UBIFS. IMA uses the inode's
i_version field to detect changes on inodes. A proper i_version support
needs to make the i_version persistent on disk, although IMA itself doesn't
need a persistent i_version. Last time an earlier version of this patch
was sent by Oleksij Rempel Richard said:

> What about making i_version persistent?
> We still have some empty fields in UBIFS' inode data structure.
> But first we have to be very sure that we need it.

This patch exactly implements this suggestion, leaving the question if we
really need it. I added the IMA maintainers to Cc in the hope that Mimi or
Dmitry can give a good reason why there's no alternative to i_version for IMA.

Sascha

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 417fe0b29f23..addf161df313 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -195,6 +195,13 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	return inode;
 }
 
+static void ubifs_dir_update_time(struct inode *dir, struct timespec time)
+{
+	dir->i_mtime = dir->i_ctime = time;
+
+	inode_inc_iversion(dir);
+}
+
 static int dbg_check_name(const struct ubifs_info *c,
 			  const struct ubifs_dent_node *dent,
 			  const struct fscrypt_name *nm)
@@ -356,7 +363,8 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	mutex_lock(&dir_ui->ui_mutex);
 	dir->i_size += sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
+
 	err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
 	if (err)
 		goto out_cancel;
@@ -770,7 +778,7 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 	inode->i_ctime = current_time(inode);
 	dir->i_size += sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
 	if (err)
 		goto out_cancel;
@@ -846,7 +854,7 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry)
 	drop_nlink(inode);
 	dir->i_size -= sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 1, 0);
 	if (err)
 		goto out_cancel;
@@ -951,7 +959,7 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry)
 	drop_nlink(dir);
 	dir->i_size -= sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 1, 0);
 	if (err)
 		goto out_cancel;
@@ -1023,7 +1031,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	inc_nlink(dir);
 	dir->i_size += sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
 	if (err) {
 		ubifs_err(c, "cannot create directory, error %d", err);
@@ -1114,7 +1122,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
 	mutex_lock(&dir_ui->ui_mutex);
 	dir->i_size += sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
 	if (err)
 		goto out_cancel;
@@ -1245,7 +1253,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
 	mutex_lock(&dir_ui->ui_mutex);
 	dir->i_size += sz_change;
 	dir_ui->ui_size = dir->i_size;
-	dir->i_mtime = dir->i_ctime = inode->i_ctime;
+	ubifs_dir_update_time(dir, inode->i_ctime);
 	err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0);
 	if (err)
 		goto out_cancel;
@@ -1450,8 +1458,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	old_dir->i_size -= old_sz;
 	ubifs_inode(old_dir)->ui_size = old_dir->i_size;
-	old_dir->i_mtime = old_dir->i_ctime = time;
-	new_dir->i_mtime = new_dir->i_ctime = time;
+	ubifs_dir_update_time(old_dir, time);
+	ubifs_dir_update_time(new_dir, time);
 
 	/*
 	 * And finally, if we unlinked a direntry which happened to have the
@@ -1595,8 +1603,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	time = current_time(old_dir);
 	fst_inode->i_ctime = time;
 	snd_inode->i_ctime = time;
-	old_dir->i_mtime = old_dir->i_ctime = time;
-	new_dir->i_mtime = new_dir->i_ctime = time;
+	ubifs_dir_update_time(old_dir, time);
+	ubifs_dir_update_time(new_dir, time);
 
 	if (old_dir != new_dir) {
 		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8cad0b19b404..54f58172b6e7 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1104,6 +1104,7 @@ static void do_attr_changes(struct inode *inode, const struct iattr *attr)
 			mode &= ~S_ISGID;
 		inode->i_mode = mode;
 	}
+	inode_inc_iversion(inode);
 }
 
 /**
@@ -1409,6 +1410,8 @@ int ubifs_update_time(struct inode *inode, struct timespec *time,
 	if (!(inode->i_sb->s_flags & MS_LAZYTIME))
 		iflags |= I_DIRTY_SYNC;
 
+	inode_inc_iversion(inode);
+
 	release = ui->dirty;
 	__mark_inode_dirty(inode, iflags);
 	mutex_unlock(&ui->ui_mutex);
@@ -1443,6 +1446,7 @@ static int update_mctime(struct inode *inode)
 
 		mutex_lock(&ui->ui_mutex);
 		inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode_inc_iversion(inode);
 		release = ui->dirty;
 		mark_inode_dirty_sync(inode);
 		mutex_unlock(&ui->ui_mutex);
@@ -1588,6 +1592,7 @@ static int ubifs_vm_page_mkwrite(struct vm_fault *vmf)
 
 		mutex_lock(&ui->ui_mutex);
 		inode->i_mtime = inode->i_ctime = current_time(inode);
+		inode_inc_iversion(inode);
 		release = ui->dirty;
 		mark_inode_dirty_sync(inode);
 		mutex_unlock(&ui->ui_mutex);
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 04c4ec6483e5..9a2062d57bc8 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -67,7 +67,7 @@
 static inline void zero_ino_node_unused(struct ubifs_ino_node *ino)
 {
 	memset(ino->padding1, 0, 4);
-	memset(ino->padding2, 0, 26);
+	memset(ino->padding2, 0, 18);
 }
 
 /**
@@ -459,6 +459,7 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino,
 	ino->ctime_nsec = cpu_to_le32(inode->i_ctime.tv_nsec);
 	ino->mtime_sec  = cpu_to_le64(inode->i_mtime.tv_sec);
 	ino->mtime_nsec = cpu_to_le32(inode->i_mtime.tv_nsec);
+	ino->iversion = cpu_to_le64(inode->i_version);
 	ino->uid   = cpu_to_le32(i_uid_read(inode));
 	ino->gid   = cpu_to_le32(i_gid_read(inode));
 	ino->mode  = cpu_to_le32(inode->i_mode);
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index bffadbb67e47..545c268033cc 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -143,6 +143,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
 	inode->i_ctime.tv_nsec = le32_to_cpu(ino->ctime_nsec);
 	inode->i_mode = le32_to_cpu(ino->mode);
 	inode->i_size = le64_to_cpu(ino->size);
+	inode->i_version = le64_to_cpu(ino->iversion);
 
 	ui->data_len    = le32_to_cpu(ino->data_len);
 	ui->flags       = le32_to_cpu(ino->flags);
@@ -2056,6 +2057,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &ubifs_super_operations;
 	sb->s_xattr = ubifs_xattr_handlers;
 	sb->s_cop = &ubifs_crypt_operations;
+	sb->s_flags |= MS_I_VERSION;
 
 	mutex_lock(&c->umount_mutex);
 	err = mount_ubifs(c);
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index e8c23c9d4f4a..d4689c7e9df5 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -525,7 +525,8 @@ struct ubifs_ino_node {
 	__u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */
 	__le32 xattr_names;
 	__le16 compr_type;
-	__u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */
+	__le64 iversion;
+	__u8 padding2[18]; /* Watch 'zero_ino_node_unused()' if changing! */
 	__u8 data[];
 } __packed;
 
-- 
2.11.0

Re: [Linux-ima-devel] Write a file into nfs will get stucked

[Rock L. <roc...@gm...>]

On Tue, Sep 12, 2017 at 4:31 PM, Rock Lee <roc...@gm...> wrote:
> Hi,
>
> I enabled IMA, but when I write a file into nfs, the process will get
> stucked. I've trace the code, it seems it never return in
> ima_calc_file_shash(). Could IMA work with nfs ??
>
> BTW, I am using raspberrypi3, linux-4.13, with IMA enabled. And my
> kernel cmdline uses "ima_tcb ima_appraise=fix ima_appraise_tcb".
>

Actually, the process get stucked at ima_check_last_writer() ->
ima_update_xattr() -> ima_collect_measurement() ->
ima_calc_file_hash() -> ima_calc_file_shash()  ->
ima_calc_file_hash_tfm()->integrity_kernel_read().
ima_check_last_writer() holds inode_lock, I suppose the read function
in nfs may also hold inode_lock. Since after I unlocked inode lock in
ima_check_last_writer()  before  ima_update_xattr(), process won't be
stucked.

-- 
Cheers,
Rock