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

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

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

On Thu, Sep 14, 2017 at 9:58 PM, Mimi Zohar <zo...@li...> wrote:
> This patch constifies the path argument to kernel_read_file_from_path.

I've applied this upstream independently of everything else, because
it's obviously the right thing to do (as the sound_firmware.h part of
the patch shows, never mind the whole "we're just passing the pathname
to filp_open() which takes a const char"  thing).

               lINUS

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

On Fri, Sep 15, 2017 at 2:04 AM, Mimi Zohar <zo...@li...> wrote:
> On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
>> This is still wrong.
>>
>> (a) there is no explanation for why we need that exclusive lock in the
>> first place
>
>> Why should a read need exclusive access? You'd think shared is sufficient.
>
> True, reading a file shouldn't require an exclusive lock.  The
> exclusive lock is taken to prevent the file from changing while the
> file hash is being calculated.

That really shouldn't need an exclusive lock either. The whole point
is that you're just reading the file, so a shared lock should be fine.

There may be other *higher* level reasons why the caller then might
want an exclusive lock for other reasons, but that should have nothing
to do with the reading part.

So this is the thing I want explained. Right now there are no
explanations, and the few comments there are about exclusive locking
don't make sense, and don't match the lockdep tests.

So the patch itself may be fine, but the commentary and explanations
are broken and/or missing.

                     Linus

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

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

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.

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

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

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...>:
> >On Fri, 2017-09-15 at 10:36 -0700, Jarkko Sakkinen wrote:
> >> On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
> >> > On Fri, 2017-09-15 at 10:18 -0700, Jarkko Sakkinen wrote:
> >[]
> >> > > We decided to create a new mailing list
> >lin...@vg...
> >> > > to cover both TPM and IMA since they tend to have cross
> >dependencies.
> >[]
> >> > And is there to be an update to the MAINTAINERS file entries?
> >[]
> >> Yes, I'll get onto it. BTW, do we need two entries for TPM in the
> >> MAINTAINERS file or can I merge those?
> >
> >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?

/JArkko

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

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

Am 15. September 2017 10:40:14 GMT-07:00 schrieb Joe Perches <jo...@pe...>:
>On Fri, 2017-09-15 at 10:36 -0700, Jarkko Sakkinen wrote:
>> On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
>> > On Fri, 2017-09-15 at 10:18 -0700, Jarkko Sakkinen wrote:
>[]
>> > > We decided to create a new mailing list
>lin...@vg...
>> > > to cover both TPM and IMA since they tend to have cross
>dependencies.
>[]
>> > And is there to be an update to the MAINTAINERS file entries?
>[]
>> Yes, I'll get onto it. BTW, do we need two entries for TPM in the
>> MAINTAINERS file or can I merge those?
>
>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
>
>
>------------------------------------------------------------------------------
>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] New ML for TPM and IMA

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

On Fri, 2017-09-15 at 10:36 -0700, Jarkko Sakkinen wrote:
> On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
> > On Fri, 2017-09-15 at 10:18 -0700, Jarkko Sakkinen wrote:
[]
> > > We decided to create a new mailing list lin...@vg...
> > > to cover both TPM and IMA since they tend to have cross dependencies.
[]
> > And is there to be an update to the MAINTAINERS file entries?
[]
> Yes, I'll get onto it. BTW, do we need two entries for TPM in the
> MAINTAINERS file or can I merge those?

As the individual maintainers are different for the two sections,
I think you need both entries.

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

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

On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
> On Fri, 2017-09-15 at 10:18 -0700, 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.
> 
> And is there to be an update to the MAINTAINERS file entries?
> 
> Extended Verification Module (EVM)
> L:	lin...@li...
> L:	lin...@vg...
> 
> INTEGRITY MEASUREMENT ARCHITECTURE (IMA)
> L:	lin...@li...
> L:	lin...@li...
> L:	lin...@vg...
> 
> TPM DEVICE DRIVER
> L:	tpm...@li... (moderated for non-subscribers)
> 
> TPM IBM_VTPM DEVICE DRIVER
> L:	tpm...@li... (moderated for non-subscribers)

Yes, I'll get onto it. BTW, do we need two entries for TPM in the
MAINTAINERS file or can I merge those?

/Jarkko

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

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

On Fri, Sep 15, 2017 at 10:21:16AM -0700, Peter Huewe wrote:
> 
> 
> Am 15. September 2017 10:18:25 GMT-07:00 schrieb Jarkko Sakkinen <jar...@li...>:
> >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...
> 
> Of course it's
> lin...@vg... 
> (Cc'ed)

D'oh, yes :-)

/Jarkko

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

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

On Fri, 2017-09-15 at 10:18 -0700, 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.

And is there to be an update to the MAINTAINERS file entries?

Extended Verification Module (EVM)
L:	lin...@li...
L:	lin...@vg...

INTEGRITY MEASUREMENT ARCHITECTURE (IMA)
L:	lin...@li...
L:	lin...@li...
L:	lin...@vg...

TPM DEVICE DRIVER
L:	tpm...@li... (moderated for non-subscribers)

TPM IBM_VTPM DEVICE DRIVER
L:	tpm...@li... (moderated for non-subscribers)

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

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

Am 15. September 2017 10:18:25 GMT-07:00 schrieb Jarkko Sakkinen <jar...@li...>:
>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...

Of course it's
lin...@vg... 
(Cc'ed)




>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.
>
>Thank you.
>
>/Jarkko
>
>------------------------------------------------------------------------------
>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

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

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

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.

Thank you.

/Jarkko

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

On Fri, 2017-09-15 at 07:49 -0700, Christoph Hellwig wrote:
> On Thu, Sep 14, 2017 at 10:50:27PM -0700, Linus Torvalds wrote:
> > This is still wrong.
> > 
> > (a) there is no explanation for why we need that exclusive lock in the
> > first place
> > 
> > Why should a read need exclusive access? You'd think shared is sufficient.
> > But regardless, it needs *explanation*.
> 
> Shared is sufficient, and nothing in the patch (except for the
> description) actually requires an exclusive lock.  It just happens that
> ima holds it exclusive for other internal reasons.

Although reading the file to calculate the file hash doesn't require
taking the lock exclusively, in either "fix" mode or called from
__fput, immediately after calculating the file hash, the file hash is
written out as an xattr.  Writing the xattr requires taking the lock
exclusively.

Mimi

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 Fri, Sep 15, 2017 at 06:07:26PM +0530, Nayna Jain wrote:
> 
> 
> On 09/13/2017 06:28 AM, Jarkko Sakkinen wrote:
> > 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.
> 
> I think I didn't understand the meaning of "get rid of wait_for_tpm_stat()".
> Do you mean to take care of it in driver specific file ?
> Can you please elaborate it ?
> 
> Thanks & Regards,
>     - Nayna

It's not a generic function. It's used only in tpm_tis and tpm-xenfront.
They should have their owen private functions for this. Here the sharing
of code makes zero sense.

/Jarkko

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

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

On Fri, Sep 15, 2017 at 05:59:21PM +0530, Nayna Jain wrote:
> 
> 
> On 09/14/2017 04:40 AM, Jarkko Sakkinen wrote:
> > 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.
> 
> It sounds like we are getting in direction to have this change by default.
> Before removing the ignore_burst_count parameter, I will post a test version of this
> patch which enables ignore_burst_count by default, for testing purposes only.
> 
> Thanks Peter and Alex for testing.
> 
> Thanks & Regards,
>    - Nayna

I could apply it immediately after some testing to my next branch where
it gets pulled to linux-next. There's still a lot of time before next
pull request so many people would get exposed. If it cause problems,
we reconsider.

/Jarkko

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

On Thu, Sep 14, 2017 at 10:50:27PM -0700, Linus Torvalds wrote:
> This is still wrong.
> 
> (a) there is no explanation for why we need that exclusive lock in the
> first place
> 
> Why should a read need exclusive access? You'd think shared is sufficient.
> But regardless, it needs *explanation*.

Shared is sufficient, and nothing in the patch (except for the
description) actually requires an exclusive lock.  It just happens that
ima holds it exclusive for other internal reasons.

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

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

On 09/13/2017 06:30 AM, Jarkko Sakkinen wrote:
> 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?

As per ddwg input, the command may not take more than a few
microseconds. The minimum tpm_msleep() value is 1 msec, so we really
don't have a choice.  (We're working on a patch set to lower this
value even more.)

Thanks & Regards,
     - Nayna

>
> /Jarkko
>

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

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

On 09/13/2017 06:28 AM, Jarkko Sakkinen wrote:
> 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.

I think I didn't understand the meaning of "get rid of wait_for_tpm_stat()".
Do you mean to take care of it in driver specific file ?
Can you please elaborate it ?

Thanks & Regards,
     - Nayna

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

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

On 09/14/2017 04:40 AM, Jarkko Sakkinen wrote:
> 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.

It sounds like we are getting in direction to have this change by default.
Before removing the ignore_burst_count parameter, I will post a test version of this
patch which enables ignore_burst_count by default, for testing purposes only.

Thanks Peter and Alex for testing.

Thanks & Regards,
    - Nayna


>
> /Jarkko
>

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

On Fri, 2017-09-15 at 05:04 -0400, Mimi Zohar wrote:
> On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
> > This is still wrong.
> > 
> > (a) there is no explanation for why we need that exclusive lock in the
> > first place
> 
> > Why should a read need exclusive access? You'd think shared is sufficient.
> 
> True, reading a file shouldn't require an exclusive lock.  The
> exclusive lock is taken to prevent the file from changing while the
> file hash is being calculated.

And also to prevent the file hash from being calculated multiple
times.

> 
> > But regardless, it needs *explanation*.
> 
> Agreed.  A fuller explanation was included in the cover letter that
> should have also been included in the patch description.  The
> following is taken from the cover letter:
> 
> With the introduction of IMA-appraisal and the need to write file
> hashes as security xattrs, IMA needed to take the global i_mutex
> lock.  process_measurement() took the iint->mutex first and then
> the i_mutex, while setxattr, chmod and chown took the locks in
> reverse order.  To resolve this potential deadlock, the iint->mutex
> was removed.
> 
> Some filesystems have recently replaced their filesystem dependent
> lock with the global i_rwsem (formerly the i_mutex) 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 set defines a new
> ->integrity_read file operation method.
> 
> (Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
> the VFS inode instead")
> 
> Mimi
> 
> > (b) the lockdep test isn't for the exclusive lock that the code comment
> > says it is
> > 
> > So no, this needs more work.
> > 
> >     Linus
> > 
> 
> > 
> > On Sep 14, 2017 21:59, "Mimi Zohar" <zo...@li...> wrote:
> > 
> > > From: Christoph Hellwig <hc...@ls...>
> > >
> > > Add a new ->integrity_read file operation to read data for integrity
> > > hash collection.  This is defined to be equivalent to ->read_iter,
> > > except that it will be called with the i_rwsem held exclusively.
> > >
> > > (Based on Christoph's original patch.)
> > >
> > > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > > Cc: Matthew Garrett <mj...@sr...>
> > > Cc: Jan Kara <ja...@su...>
> > > Cc: "Theodore Ts'o" <ty...@mi...>
> > > Cc: Andreas Dilger <adi...@di...>
> > > Cc: Jaegeuk Kim <ja...@ke...>
> > > Cc: Chao Yu <yu...@hu...>
> > > Cc: Steven Whitehouse <swh...@re...>
> > > Cc: Bob Peterson <rpe...@re...>
> > > Cc: David Woodhouse <dw...@in...>
> > > Cc: Dave Kleikamp <sh...@ke...>
> > > Cc: Ryusuke Konishi <kon...@la...>
> > > Cc: Mark Fasheh <mf...@ve...>
> > > Cc: Joel Becker <jl...@ev...>
> > > Cc: Richard Weinberger <ri...@no...>
> > > Cc: "Darrick J. Wong" <dar...@or...>
> > > Cc: Hugh Dickins <hu...@go...>
> > > Cc: Chris Mason <cl...@fb...>
> > > Signed-off-by: Mimi Zohar <zo...@li...>
> > > Reviewed-by: Jan Kara <ja...@su...>
> > > Reviewed-by: Dmitry Kasatkin <dmi...@hu...>
> > > ---
> > >  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
> > >
> > >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

On Thu, 2017-09-14 at 22:50 -0700, Linus Torvalds wrote:
> This is still wrong.
> 
> (a) there is no explanation for why we need that exclusive lock in the
> first place

> Why should a read need exclusive access? You'd think shared is sufficient.

True, reading a file shouldn't require an exclusive lock.  The
exclusive lock is taken to prevent the file from changing while the
file hash is being calculated.

> But regardless, it needs *explanation*.

Agreed.  A fuller explanation was included in the cover letter that
should have also been included in the patch description.  The
following is taken from the cover letter:

With the introduction of IMA-appraisal and the need to write file
hashes as security xattrs, IMA needed to take the global i_mutex
lock.  process_measurement() took the iint->mutex first and then
the i_mutex, while setxattr, chmod and chown took the locks in
reverse order.  To resolve this potential deadlock, the iint->mutex
was removed.

Some filesystems have recently replaced their filesystem dependent
lock with the global i_rwsem (formerly the i_mutex) 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 set defines a new
->integrity_read file operation method.

(Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in
the VFS inode instead")

Mimi

> (b) the lockdep test isn't for the exclusive lock that the code comment
> says it is
> 
> So no, this needs more work.
> 
>     Linus
> 

> 
> On Sep 14, 2017 21:59, "Mimi Zohar" <zo...@li...> wrote:
> 
> > From: Christoph Hellwig <hc...@ls...>
> >
> > Add a new ->integrity_read file operation to read data for integrity
> > hash collection.  This is defined to be equivalent to ->read_iter,
> > except that it will be called with the i_rwsem held exclusively.
> >
> > (Based on Christoph's original patch.)
> >
> > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > Cc: Matthew Garrett <mj...@sr...>
> > Cc: Jan Kara <ja...@su...>
> > Cc: "Theodore Ts'o" <ty...@mi...>
> > Cc: Andreas Dilger <adi...@di...>
> > Cc: Jaegeuk Kim <ja...@ke...>
> > Cc: Chao Yu <yu...@hu...>
> > Cc: Steven Whitehouse <swh...@re...>
> > Cc: Bob Peterson <rpe...@re...>
> > Cc: David Woodhouse <dw...@in...>
> > Cc: Dave Kleikamp <sh...@ke...>
> > Cc: Ryusuke Konishi <kon...@la...>
> > Cc: Mark Fasheh <mf...@ve...>
> > Cc: Joel Becker <jl...@ev...>
> > Cc: Richard Weinberger <ri...@no...>
> > Cc: "Darrick J. Wong" <dar...@or...>
> > Cc: Hugh Dickins <hu...@go...>
> > Cc: Chris Mason <cl...@fb...>
> > Signed-off-by: Mimi Zohar <zo...@li...>
> > Reviewed-by: Jan Kara <ja...@su...>
> > Reviewed-by: Dmitry Kasatkin <dmi...@hu...>
> > ---
> >  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] [PATCH 3/3] ima: use fs method to read integrity data

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

This is still wrong.

(a) there is no explanation for why we need that exclusive lock in the
first place

Why should a read need exclusive access? You'd think shared is sufficient.
But regardless, it needs *explanation*.

(b) the lockdep test isn't for the exclusive lock that the code comment
says it is

So no, this needs more work.

    Linus



On Sep 14, 2017 21:59, "Mimi Zohar" <zo...@li...> wrote:

> From: Christoph Hellwig <hc...@ls...>
>
> Add a new ->integrity_read file operation to read data for integrity
> hash collection.  This is defined to be equivalent to ->read_iter,
> except that it will be called with the i_rwsem held exclusively.
>
> (Based on Christoph's original patch.)
>
> Signed-off-by: Christoph Hellwig <hc...@ls...>
> Cc: Matthew Garrett <mj...@sr...>
> Cc: Jan Kara <ja...@su...>
> Cc: "Theodore Ts'o" <ty...@mi...>
> Cc: Andreas Dilger <adi...@di...>
> Cc: Jaegeuk Kim <ja...@ke...>
> Cc: Chao Yu <yu...@hu...>
> Cc: Steven Whitehouse <swh...@re...>
> Cc: Bob Peterson <rpe...@re...>
> Cc: David Woodhouse <dw...@in...>
> Cc: Dave Kleikamp <sh...@ke...>
> Cc: Ryusuke Konishi <kon...@la...>
> Cc: Mark Fasheh <mf...@ve...>
> Cc: Joel Becker <jl...@ev...>
> Cc: Richard Weinberger <ri...@no...>
> Cc: "Darrick J. Wong" <dar...@or...>
> Cc: Hugh Dickins <hu...@go...>
> Cc: Chris Mason <cl...@fb...>
> Signed-off-by: Mimi Zohar <zo...@li...>
> Reviewed-by: Jan Kara <ja...@su...>
> Reviewed-by: Dmitry Kasatkin <dmi...@hu...>
> ---
>  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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...

[Linux-ima-devel] [PATCH 3/3] ima: use fs method to read integrity data

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

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

Add a new ->integrity_read file operation to read data for integrity
hash collection.  This is defined to be equivalent to ->read_iter,
except that it will be called with the i_rwsem held exclusively.

(Based on Christoph's original patch.)

Signed-off-by: Christoph Hellwig <hc...@ls...>
Cc: Matthew Garrett <mj...@sr...>
Cc: Jan Kara <ja...@su...>
Cc: "Theodore Ts'o" <ty...@mi...>
Cc: Andreas Dilger <adi...@di...>
Cc: Jaegeuk Kim <ja...@ke...>
Cc: Chao Yu <yu...@hu...>
Cc: Steven Whitehouse <swh...@re...>
Cc: Bob Peterson <rpe...@re...>
Cc: David Woodhouse <dw...@in...>
Cc: Dave Kleikamp <sh...@ke...>
Cc: Ryusuke Konishi <kon...@la...>
Cc: Mark Fasheh <mf...@ve...>
Cc: Joel Becker <jl...@ev...>
Cc: Richard Weinberger <ri...@no...>
Cc: "Darrick J. Wong" <dar...@or...>
Cc: Hugh Dickins <hu...@go...>
Cc: Chris Mason <cl...@fb...>
Signed-off-by: Mimi Zohar <zo...@li...>
Reviewed-by: Jan Kara <ja...@su...>
Reviewed-by: Dmitry Kasatkin <dmi...@hu...>
---
 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

[Linux-ima-devel] [PATCH 2/3] 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/3] 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.

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

[Linux-ima-devel] [PATCH 0/3] ima: only call integrity_kernel_read to calc file hash

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

The integrity_kernel_read() function was originally introduced to
read a file and calculate the file hash by-passing any security
checks.  Support subsequently was added allowing the kernel to read
a file containing a signed x509 certificate and load it onto either
the IMA or EVM keyring.

This patch set replaces the call to integrity_kernel_read() with the
common kernel_read_file_from_path() function, for reading and
loading an x509 certificate onto either the IMA or EVM keyring.  The
remaining calls to integrity_kernel_read() calculate a file hash, by
calling the new integrity_read file operation method.

Mimi


Christoph Hellwig (2):
  integrity: replace call to integrity_read_file with kernel version
  ima: use fs method to read integrity data

Mimi Zohar (1):
  vfs: constify path argument to kernel_read_file_from_path

 fs/btrfs/file.c                   |  1 +
 fs/efivarfs/file.c                |  1 +
 fs/exec.c                         |  2 +-
 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                |  4 ++-
 mm/shmem.c                        |  1 +
 security/integrity/digsig.c       | 14 +++++---
 security/integrity/iint.c         | 69 ++++++++-------------------------------
 security/integrity/ima/ima_main.c |  4 +++
 security/integrity/integrity.h    |  2 --
 sound/oss/sound_firmware.h        |  2 +-
 20 files changed, 100 insertions(+), 65 deletions(-)

-- 
2.7.4