Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Mimi Zohar]

On Wed, 2018-01-03 at 14:16 +1100, Dave Chinner wrote:
> On Tue, Jan 02, 2018 at 09:52:03PM -0500, Mimi Zohar wrote:
> > On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> > > [might as well cc linux-xfs]
> > > 
> > > On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > > > Hi,
> > > > 
> > > > Could I ask FS maintainers to test IMA with this patch additionally
> > > > and provide ack/tested.
> > > > We tested but may be you have and some special testing.
> > > 
> > > Super-late to this party, but unless xfstests has automated tests to
> > > set up IMA on top of an existing filesystem then I most likely have no
> > > idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> > > see anything IMA-related.
> > 
> > Back in June I posted a simple xfstests IMA-appraisal test (https://ma
> > rc.info/?l=linux-fsdevel=149703820814885=4).
> 
> That's a really, really basic test and it doesn't exercise the
> problematic direct IO path this patch fixes problems with. nor does
> it exercise the chmod path, or try to trigger deadlocks or other
> conditions through all the other paths that can trigger IMA actions
> and or failures (e.g. ENOSPC).  IOWs, we need a lot more than a
> "hello world" test to be able to verify filesystems interact with
> IMA properly. e.g. how does it behave at ENOSPC?

True, but for now we were looking for some basic testing - opening a
file and calculating the file hash - on different filesystems, not the
direct-IO path in particular.  Expanding the IMA-appraisal xfstests is
high on my "todo" list.

Mimi

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Mimi Zohar]

On Wed, 2018-01-03 at 14:16 +1100, Dave Chinner wrote:
> On Tue, Jan 02, 2018 at 09:52:03PM -0500, Mimi Zohar wrote:
> > On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> > > [might as well cc linux-xfs]
> > > 
> > > On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > > > Hi,
> > > > 
> > > > Could I ask FS maintainers to test IMA with this patch additionally
> > > > and provide ack/tested.
> > > > We tested but may be you have and some special testing.
> > > 
> > > Super-late to this party, but unless xfstests has automated tests to
> > > set up IMA on top of an existing filesystem then I most likely have no
> > > idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> > > see anything IMA-related.
> > 
> > Back in June I posted a simple xfstests IMA-appraisal test (https://ma
> > rc.info/?l=linux-fsdevel=149703820814885=4).
> 
> That's a really, really basic test and it doesn't exercise the
> problematic direct IO path this patch fixes problems with. nor does
> it exercise the chmod path, or try to trigger deadlocks or other
> conditions through all the other paths that can trigger IMA actions
> and or failures (e.g. ENOSPC).  IOWs, we need a lot more than a
> "hello world" test to be able to verify filesystems interact with
> IMA properly. e.g. how does it behave at ENOSPC?

True, but for now we were looking for some basic testing - opening a
file and calculating the file hash - on different filesystems, not the
direct-IO path in particular.  Expanding the IMA-appraisal xfstests is
high on my "todo" list.

Mimi

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dave Chinner]

On Tue, Jan 02, 2018 at 09:52:03PM -0500, Mimi Zohar wrote:
> On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> > [might as well cc linux-xfs]
> > 
> > On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > > Hi,
> > > 
> > > Could I ask FS maintainers to test IMA with this patch additionally
> > > and provide ack/tested.
> > > We tested but may be you have and some special testing.
> > 
> > Super-late to this party, but unless xfstests has automated tests to
> > set up IMA on top of an existing filesystem then I most likely have no
> > idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> > see anything IMA-related.
> 
> Back in June I posted a simple xfstests IMA-appraisal test (https://ma
> rc.info/?l=linux-fsdevel=149703820814885=4).

That's a really, really basic test and it doesn't exercise the
problematic direct IO path this patch fixes problems with. nor does
it exercise the chmod path, or try to trigger deadlocks or other
conditions through all the other paths that can trigger IMA actions
and or failures (e.g. ENOSPC).  IOWs, we need a lot more than a
"hello world" test to be able to verify filesystems interact with
IMA properly. e.g. how does it behave at ENOSPC? 

How do you test that IMA is fully working and has no regressions
during your development?  I'm sure there's more than a "hello world"
test for that

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dave Chinner]

On Tue, Jan 02, 2018 at 09:52:03PM -0500, Mimi Zohar wrote:
> On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> > [might as well cc linux-xfs]
> > 
> > On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > > Hi,
> > > 
> > > Could I ask FS maintainers to test IMA with this patch additionally
> > > and provide ack/tested.
> > > We tested but may be you have and some special testing.
> > 
> > Super-late to this party, but unless xfstests has automated tests to
> > set up IMA on top of an existing filesystem then I most likely have no
> > idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> > see anything IMA-related.
> 
> Back in June I posted a simple xfstests IMA-appraisal test (https://ma
> rc.info/?l=linux-fsdevel=149703820814885=4).

That's a really, really basic test and it doesn't exercise the
problematic direct IO path this patch fixes problems with. nor does
it exercise the chmod path, or try to trigger deadlocks or other
conditions through all the other paths that can trigger IMA actions
and or failures (e.g. ENOSPC).  IOWs, we need a lot more than a
"hello world" test to be able to verify filesystems interact with
IMA properly. e.g. how does it behave at ENOSPC? 

How do you test that IMA is fully working and has no regressions
during your development?  I'm sure there's more than a "hello world"
test for that

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Mimi Zohar]

On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> [might as well cc linux-xfs]
> 
> On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > Hi,
> > 
> > Could I ask FS maintainers to test IMA with this patch additionally
> > and provide ack/tested.
> > We tested but may be you have and some special testing.
> 
> Super-late to this party, but unless xfstests has automated tests to
> set up IMA on top of an existing filesystem then I most likely have no
> idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> see anything IMA-related.

Back in June I posted a simple xfstests IMA-appraisal test (https://ma
rc.info/?l=linux-fsdevel=149703820814885=4).

Mimi

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Mimi Zohar]

On Tue, 2018-01-02 at 17:40 -0800, Darrick J. Wong wrote:
> [might as well cc linux-xfs]
> 
> On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> > Hi,
> > 
> > Could I ask FS maintainers to test IMA with this patch additionally
> > and provide ack/tested.
> > We tested but may be you have and some special testing.
> 
> Super-late to this party, but unless xfstests has automated tests to
> set up IMA on top of an existing filesystem then I most likely have no
> idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
> see anything IMA-related.

Back in June I posted a simple xfstests IMA-appraisal test (https://ma
rc.info/?l=linux-fsdevel=149703820814885=4).

Mimi

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Darrick J. Wong]

[might as well cc linux-xfs]

On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> Could I ask FS maintainers to test IMA with this patch additionally
> and provide ack/tested.
> We tested but may be you have and some special testing.

Super-late to this party, but unless xfstests has automated tests to
set up IMA on top of an existing filesystem then I most likely have no
idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
see anything IMA-related.

--D

> 
> Thanks in advance,
> Dmitry
> 
> On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin
>  wrote:
> > The original design was discussed 3+ years ago, but was never 
> > completed/upstreamed.
> > Based on the recent discussions with Linus
> > https://patchwork.kernel.org/patch/9975919, I've rebased this patch.
> >
> > Before IMA appraisal was introduced, IMA was using own integrity cache
> > lock along with i_mutex. process_measurement and ima_file_free took
> > the iint->mutex first and then the i_mutex, while setxattr, chmod and
> > chown took the locks in reverse order. To resolve the potential deadlock,
> > i_mutex was moved to protect entire IMA functionality and the redundant
> > iint->mutex was eliminated.
> >
> > Solution was based on the assumption that filesystem code does not take
> > i_mutex further. But when file is opened with O_DIRECT flag, direct-io
> > implementation takes i_mutex and produces deadlock. Furthermore, certain
> > other filesystem operations, such as llseek, also take i_mutex.
> >
> > More recently some filesystems have replaced their filesystem specific
> > lock with the global i_rwsem to read a file.  As a result, when IMA
> > attempts to calculate the file hash, reading the file attempts to take
> > the i_rwsem again.
> >
> > To resolve O_DIRECT related deadlock problem, this patch re-introduces
> > iint->mutex. But to eliminate the original chmod() related deadlock
> > problem, this patch eliminates the requirement for chmod hooks to take
> > the iint->mutex by introducing additional atomic iint->attr_flags to
> > indicate calling of the hooks. The allowed locking order is to take
> > the iint->mutex first and then the i_rwsem.
> >
> > Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
> > and tested when file was closed or opened again. New atomic flags are set
> > or cleared in those hooks and tested to clear iint->flags on close or on 
> > open.
> >
> > Atomic flags are following:
> > * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
> >   and file attributes have changed. On file open, it causes IMA to clear
> >   iint->flags to re-evaluate policy and perform IMA functions again.
> > * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
> >   extended attributes have changed. On file open, it causes IMA to clear
> >   iint->flags IMA_DONE_MASK to re-appraise.
> > * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
> >   It is cleared if file policy changes and no update is needed.
> > * IMA_DIGSIG - indicates that file security.ima has signature and file
> >   security.ima must not update to file has on file close.
> > * IMA_MUST_MEASURE - indicates the file is in the measurement policy.
> >
> > Changes in v6:
> > * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
> >   the measurement policy. It is used instead of the IMA_MEASURE 
> > (iint->flags)
> >   to detect ToMToU violation and when iint->mutex is unlocked and behind 
> > inode
> >   lock only (same as some other flags). Issue reported by Roberto Sassu.
> >
> > Changes in v5:
> > * use of inode_lock() and inode_unlock()
> >
> > Changes in v4:
> > * adoped to violation detection fixes
> > * added IMA_UPDATE_XATTR flag to require xattr update on file close
> >
> > Changes in v3:
> > * prevent signature removal with new locking
> > * rename attr_flags to atomic_flags
> >
> > Changes in v2:
> > * revert taking the i_mutex in integrity_inode_get() so that iint allocation
> >   could be done with i_mutex taken
> > * move taking the i_mutex from appraisal code to the process_measurement()
> >
> > Signed-off-by: Dmitry Kasatkin 
> > ---
> >  security/integrity/iint.c |  2 +
> >  security/integrity/ima/ima_appraise.c | 27 +++---
> >  security/integrity/ima/ima_main.c | 70 
> > ---
> >  security/integrity/integrity.h| 18 ++---
> >  4 files changed, 77 insertions(+), 40 deletions(-)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index c84e058..d726ba23 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -155,12 +155,14 @@ static void init_once(void *foo)
> > memset(iint, 0, sizeof(*iint));
> > iint->version = 0;
> > iint->flags = 0UL;
> > +   iint->atomic_flags = 0;
> > iint->ima_file_status = 

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Darrick J. Wong]

[might as well cc linux-xfs]

On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote:
> Hi,
> 
> Could I ask FS maintainers to test IMA with this patch additionally
> and provide ack/tested.
> We tested but may be you have and some special testing.

Super-late to this party, but unless xfstests has automated tests to
set up IMA on top of an existing filesystem then I most likely have no
idea /how/ to test IMA.  I did a quick grep of xfstests git and I don't
see anything IMA-related.

--D

> 
> Thanks in advance,
> Dmitry
> 
> On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin
>  wrote:
> > The original design was discussed 3+ years ago, but was never 
> > completed/upstreamed.
> > Based on the recent discussions with Linus
> > https://patchwork.kernel.org/patch/9975919, I've rebased this patch.
> >
> > Before IMA appraisal was introduced, IMA was using own integrity cache
> > lock along with i_mutex. process_measurement and ima_file_free took
> > the iint->mutex first and then the i_mutex, while setxattr, chmod and
> > chown took the locks in reverse order. To resolve the potential deadlock,
> > i_mutex was moved to protect entire IMA functionality and the redundant
> > iint->mutex was eliminated.
> >
> > Solution was based on the assumption that filesystem code does not take
> > i_mutex further. But when file is opened with O_DIRECT flag, direct-io
> > implementation takes i_mutex and produces deadlock. Furthermore, certain
> > other filesystem operations, such as llseek, also take i_mutex.
> >
> > More recently some filesystems have replaced their filesystem specific
> > lock with the global i_rwsem to read a file.  As a result, when IMA
> > attempts to calculate the file hash, reading the file attempts to take
> > the i_rwsem again.
> >
> > To resolve O_DIRECT related deadlock problem, this patch re-introduces
> > iint->mutex. But to eliminate the original chmod() related deadlock
> > problem, this patch eliminates the requirement for chmod hooks to take
> > the iint->mutex by introducing additional atomic iint->attr_flags to
> > indicate calling of the hooks. The allowed locking order is to take
> > the iint->mutex first and then the i_rwsem.
> >
> > Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
> > and tested when file was closed or opened again. New atomic flags are set
> > or cleared in those hooks and tested to clear iint->flags on close or on 
> > open.
> >
> > Atomic flags are following:
> > * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
> >   and file attributes have changed. On file open, it causes IMA to clear
> >   iint->flags to re-evaluate policy and perform IMA functions again.
> > * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
> >   extended attributes have changed. On file open, it causes IMA to clear
> >   iint->flags IMA_DONE_MASK to re-appraise.
> > * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
> >   It is cleared if file policy changes and no update is needed.
> > * IMA_DIGSIG - indicates that file security.ima has signature and file
> >   security.ima must not update to file has on file close.
> > * IMA_MUST_MEASURE - indicates the file is in the measurement policy.
> >
> > Changes in v6:
> > * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
> >   the measurement policy. It is used instead of the IMA_MEASURE 
> > (iint->flags)
> >   to detect ToMToU violation and when iint->mutex is unlocked and behind 
> > inode
> >   lock only (same as some other flags). Issue reported by Roberto Sassu.
> >
> > Changes in v5:
> > * use of inode_lock() and inode_unlock()
> >
> > Changes in v4:
> > * adoped to violation detection fixes
> > * added IMA_UPDATE_XATTR flag to require xattr update on file close
> >
> > Changes in v3:
> > * prevent signature removal with new locking
> > * rename attr_flags to atomic_flags
> >
> > Changes in v2:
> > * revert taking the i_mutex in integrity_inode_get() so that iint allocation
> >   could be done with i_mutex taken
> > * move taking the i_mutex from appraisal code to the process_measurement()
> >
> > Signed-off-by: Dmitry Kasatkin 
> > ---
> >  security/integrity/iint.c |  2 +
> >  security/integrity/ima/ima_appraise.c | 27 +++---
> >  security/integrity/ima/ima_main.c | 70 
> > ---
> >  security/integrity/integrity.h| 18 ++---
> >  4 files changed, 77 insertions(+), 40 deletions(-)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index c84e058..d726ba23 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -155,12 +155,14 @@ static void init_once(void *foo)
> > memset(iint, 0, sizeof(*iint));
> > iint->version = 0;
> > iint->flags = 0UL;
> > +   iint->atomic_flags = 0;
> > iint->ima_file_status = INTEGRITY_UNKNOWN;
> > iint->ima_mmap_status = 

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dmitry Kasatkin]

Hi,

Could I ask FS maintainers to test IMA with this patch additionally
and provide ack/tested.
We tested but may be you have and some special testing.

Thanks in advance,
Dmitry

On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin
 wrote:
> The original design was discussed 3+ years ago, but was never 
> completed/upstreamed.
> Based on the recent discussions with Linus
> https://patchwork.kernel.org/patch/9975919, I've rebased this patch.
>
> Before IMA appraisal was introduced, IMA was using own integrity cache
> lock along with i_mutex. process_measurement and ima_file_free took
> the iint->mutex first and then the i_mutex, while setxattr, chmod and
> chown took the locks in reverse order. To resolve the potential deadlock,
> i_mutex was moved to protect entire IMA functionality and the redundant
> iint->mutex was eliminated.
>
> Solution was based on the assumption that filesystem code does not take
> i_mutex further. But when file is opened with O_DIRECT flag, direct-io
> implementation takes i_mutex and produces deadlock. Furthermore, certain
> other filesystem operations, such as llseek, also take i_mutex.
>
> More recently some filesystems have replaced their filesystem specific
> lock with the global i_rwsem to read a file.  As a result, when IMA
> attempts to calculate the file hash, reading the file attempts to take
> the i_rwsem again.
>
> To resolve O_DIRECT related deadlock problem, this patch re-introduces
> iint->mutex. But to eliminate the original chmod() related deadlock
> problem, this patch eliminates the requirement for chmod hooks to take
> the iint->mutex by introducing additional atomic iint->attr_flags to
> indicate calling of the hooks. The allowed locking order is to take
> the iint->mutex first and then the i_rwsem.
>
> Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
> and tested when file was closed or opened again. New atomic flags are set
> or cleared in those hooks and tested to clear iint->flags on close or on open.
>
> Atomic flags are following:
> * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
>   and file attributes have changed. On file open, it causes IMA to clear
>   iint->flags to re-evaluate policy and perform IMA functions again.
> * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
>   extended attributes have changed. On file open, it causes IMA to clear
>   iint->flags IMA_DONE_MASK to re-appraise.
> * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
>   It is cleared if file policy changes and no update is needed.
> * IMA_DIGSIG - indicates that file security.ima has signature and file
>   security.ima must not update to file has on file close.
> * IMA_MUST_MEASURE - indicates the file is in the measurement policy.
>
> Changes in v6:
> * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
>   the measurement policy. It is used instead of the IMA_MEASURE (iint->flags)
>   to detect ToMToU violation and when iint->mutex is unlocked and behind inode
>   lock only (same as some other flags). Issue reported by Roberto Sassu.
>
> Changes in v5:
> * use of inode_lock() and inode_unlock()
>
> Changes in v4:
> * adoped to violation detection fixes
> * added IMA_UPDATE_XATTR flag to require xattr update on file close
>
> Changes in v3:
> * prevent signature removal with new locking
> * rename attr_flags to atomic_flags
>
> Changes in v2:
> * revert taking the i_mutex in integrity_inode_get() so that iint allocation
>   could be done with i_mutex taken
> * move taking the i_mutex from appraisal code to the process_measurement()
>
> Signed-off-by: Dmitry Kasatkin 
> ---
>  security/integrity/iint.c |  2 +
>  security/integrity/ima/ima_appraise.c | 27 +++---
>  security/integrity/ima/ima_main.c | 70 
> ---
>  security/integrity/integrity.h| 18 ++---
>  4 files changed, 77 insertions(+), 40 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c84e058..d726ba23 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -155,12 +155,14 @@ static void init_once(void *foo)
> memset(iint, 0, sizeof(*iint));
> iint->version = 0;
> iint->flags = 0UL;
> +   iint->atomic_flags = 0;
> iint->ima_file_status = INTEGRITY_UNKNOWN;
> iint->ima_mmap_status = INTEGRITY_UNKNOWN;
> iint->ima_bprm_status = INTEGRITY_UNKNOWN;
> iint->ima_read_status = INTEGRITY_UNKNOWN;
> iint->evm_status = INTEGRITY_UNKNOWN;
> iint->measured_pcrs = 0;
> +   mutex_init(>mutex);
>  }
>
>  static int __init integrity_iintcache_init(void)
> diff --git a/security/integrity/ima/ima_appraise.c 
> b/security/integrity/ima/ima_appraise.c
> index 9a54c77..3fc96dbd 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ 

Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dmitry Kasatkin]

Hi,

Could I ask FS maintainers to test IMA with this patch additionally
and provide ack/tested.
We tested but may be you have and some special testing.

Thanks in advance,
Dmitry

On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin
 wrote:
> The original design was discussed 3+ years ago, but was never 
> completed/upstreamed.
> Based on the recent discussions with Linus
> https://patchwork.kernel.org/patch/9975919, I've rebased this patch.
>
> Before IMA appraisal was introduced, IMA was using own integrity cache
> lock along with i_mutex. process_measurement and ima_file_free took
> the iint->mutex first and then the i_mutex, while setxattr, chmod and
> chown took the locks in reverse order. To resolve the potential deadlock,
> i_mutex was moved to protect entire IMA functionality and the redundant
> iint->mutex was eliminated.
>
> Solution was based on the assumption that filesystem code does not take
> i_mutex further. But when file is opened with O_DIRECT flag, direct-io
> implementation takes i_mutex and produces deadlock. Furthermore, certain
> other filesystem operations, such as llseek, also take i_mutex.
>
> More recently some filesystems have replaced their filesystem specific
> lock with the global i_rwsem to read a file.  As a result, when IMA
> attempts to calculate the file hash, reading the file attempts to take
> the i_rwsem again.
>
> To resolve O_DIRECT related deadlock problem, this patch re-introduces
> iint->mutex. But to eliminate the original chmod() related deadlock
> problem, this patch eliminates the requirement for chmod hooks to take
> the iint->mutex by introducing additional atomic iint->attr_flags to
> indicate calling of the hooks. The allowed locking order is to take
> the iint->mutex first and then the i_rwsem.
>
> Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
> and tested when file was closed or opened again. New atomic flags are set
> or cleared in those hooks and tested to clear iint->flags on close or on open.
>
> Atomic flags are following:
> * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
>   and file attributes have changed. On file open, it causes IMA to clear
>   iint->flags to re-evaluate policy and perform IMA functions again.
> * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
>   extended attributes have changed. On file open, it causes IMA to clear
>   iint->flags IMA_DONE_MASK to re-appraise.
> * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
>   It is cleared if file policy changes and no update is needed.
> * IMA_DIGSIG - indicates that file security.ima has signature and file
>   security.ima must not update to file has on file close.
> * IMA_MUST_MEASURE - indicates the file is in the measurement policy.
>
> Changes in v6:
> * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
>   the measurement policy. It is used instead of the IMA_MEASURE (iint->flags)
>   to detect ToMToU violation and when iint->mutex is unlocked and behind inode
>   lock only (same as some other flags). Issue reported by Roberto Sassu.
>
> Changes in v5:
> * use of inode_lock() and inode_unlock()
>
> Changes in v4:
> * adoped to violation detection fixes
> * added IMA_UPDATE_XATTR flag to require xattr update on file close
>
> Changes in v3:
> * prevent signature removal with new locking
> * rename attr_flags to atomic_flags
>
> Changes in v2:
> * revert taking the i_mutex in integrity_inode_get() so that iint allocation
>   could be done with i_mutex taken
> * move taking the i_mutex from appraisal code to the process_measurement()
>
> Signed-off-by: Dmitry Kasatkin 
> ---
>  security/integrity/iint.c |  2 +
>  security/integrity/ima/ima_appraise.c | 27 +++---
>  security/integrity/ima/ima_main.c | 70 
> ---
>  security/integrity/integrity.h| 18 ++---
>  4 files changed, 77 insertions(+), 40 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c84e058..d726ba23 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -155,12 +155,14 @@ static void init_once(void *foo)
> memset(iint, 0, sizeof(*iint));
> iint->version = 0;
> iint->flags = 0UL;
> +   iint->atomic_flags = 0;
> iint->ima_file_status = INTEGRITY_UNKNOWN;
> iint->ima_mmap_status = INTEGRITY_UNKNOWN;
> iint->ima_bprm_status = INTEGRITY_UNKNOWN;
> iint->ima_read_status = INTEGRITY_UNKNOWN;
> iint->evm_status = INTEGRITY_UNKNOWN;
> iint->measured_pcrs = 0;
> +   mutex_init(>mutex);
>  }
>
>  static int __init integrity_iintcache_init(void)
> diff --git a/security/integrity/ima/ima_appraise.c 
> b/security/integrity/ima/ima_appraise.c
> index 9a54c77..3fc96dbd 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -251,6 +251,7 @@ int 

[PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dmitry Kasatkin]

The original design was discussed 3+ years ago, but was never 
completed/upstreamed.
Based on the recent discussions with Linus
https://patchwork.kernel.org/patch/9975919, I've rebased this patch.

Before IMA appraisal was introduced, IMA was using own integrity cache
lock along with i_mutex. process_measurement and ima_file_free took
the iint->mutex first and then the i_mutex, while setxattr, chmod and
chown took the locks in reverse order. To resolve the potential deadlock,
i_mutex was moved to protect entire IMA functionality and the redundant
iint->mutex was eliminated.

Solution was based on the assumption that filesystem code does not take
i_mutex further. But when file is opened with O_DIRECT flag, direct-io
implementation takes i_mutex and produces deadlock. Furthermore, certain
other filesystem operations, such as llseek, also take i_mutex.

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

To resolve O_DIRECT related deadlock problem, this patch re-introduces
iint->mutex. But to eliminate the original chmod() related deadlock
problem, this patch eliminates the requirement for chmod hooks to take
the iint->mutex by introducing additional atomic iint->attr_flags to
indicate calling of the hooks. The allowed locking order is to take
the iint->mutex first and then the i_rwsem.

Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
and tested when file was closed or opened again. New atomic flags are set
or cleared in those hooks and tested to clear iint->flags on close or on open.

Atomic flags are following:
* IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
  and file attributes have changed. On file open, it causes IMA to clear
  iint->flags to re-evaluate policy and perform IMA functions again.
* IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
  extended attributes have changed. On file open, it causes IMA to clear
  iint->flags IMA_DONE_MASK to re-appraise.
* IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
  It is cleared if file policy changes and no update is needed.
* IMA_DIGSIG - indicates that file security.ima has signature and file
  security.ima must not update to file has on file close.
* IMA_MUST_MEASURE - indicates the file is in the measurement policy.

Changes in v6:
* introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
  the measurement policy. It is used instead of the IMA_MEASURE (iint->flags)
  to detect ToMToU violation and when iint->mutex is unlocked and behind inode
  lock only (same as some other flags). Issue reported by Roberto Sassu.

Changes in v5:
* use of inode_lock() and inode_unlock()

Changes in v4:
* adoped to violation detection fixes
* added IMA_UPDATE_XATTR flag to require xattr update on file close

Changes in v3:
* prevent signature removal with new locking
* rename attr_flags to atomic_flags

Changes in v2:
* revert taking the i_mutex in integrity_inode_get() so that iint allocation
  could be done with i_mutex taken
* move taking the i_mutex from appraisal code to the process_measurement()

Signed-off-by: Dmitry Kasatkin 
---
 security/integrity/iint.c |  2 +
 security/integrity/ima/ima_appraise.c | 27 +++---
 security/integrity/ima/ima_main.c | 70 ---
 security/integrity/integrity.h| 18 ++---
 4 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c84e058..d726ba23 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -155,12 +155,14 @@ static void init_once(void *foo)
memset(iint, 0, sizeof(*iint));
iint->version = 0;
iint->flags = 0UL;
+   iint->atomic_flags = 0;
iint->ima_file_status = INTEGRITY_UNKNOWN;
iint->ima_mmap_status = INTEGRITY_UNKNOWN;
iint->ima_bprm_status = INTEGRITY_UNKNOWN;
iint->ima_read_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
+   mutex_init(>mutex);
 }
 
 static int __init integrity_iintcache_init(void)
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 9a54c77..3fc96dbd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_FAIL;
break;
}
+   clear_bit(IMA_DIGSIG, >atomic_flags);
if (xattr_len - sizeof(xattr_value->type) - hash_start >=
iint->ima_hash->length)
/* xattr length may be longer. md5 hash in previous
@@ 

[PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Dmitry Kasatkin]

The original design was discussed 3+ years ago, but was never 
completed/upstreamed.
Based on the recent discussions with Linus
https://patchwork.kernel.org/patch/9975919, I've rebased this patch.

Before IMA appraisal was introduced, IMA was using own integrity cache
lock along with i_mutex. process_measurement and ima_file_free took
the iint->mutex first and then the i_mutex, while setxattr, chmod and
chown took the locks in reverse order. To resolve the potential deadlock,
i_mutex was moved to protect entire IMA functionality and the redundant
iint->mutex was eliminated.

Solution was based on the assumption that filesystem code does not take
i_mutex further. But when file is opened with O_DIRECT flag, direct-io
implementation takes i_mutex and produces deadlock. Furthermore, certain
other filesystem operations, such as llseek, also take i_mutex.

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

To resolve O_DIRECT related deadlock problem, this patch re-introduces
iint->mutex. But to eliminate the original chmod() related deadlock
problem, this patch eliminates the requirement for chmod hooks to take
the iint->mutex by introducing additional atomic iint->attr_flags to
indicate calling of the hooks. The allowed locking order is to take
the iint->mutex first and then the i_rwsem.

Original flags were cleared in chmod(), setxattr() or removwxattr() hooks
and tested when file was closed or opened again. New atomic flags are set
or cleared in those hooks and tested to clear iint->flags on close or on open.

Atomic flags are following:
* IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp)
  and file attributes have changed. On file open, it causes IMA to clear
  iint->flags to re-evaluate policy and perform IMA functions again.
* IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and
  extended attributes have changed. On file open, it causes IMA to clear
  iint->flags IMA_DONE_MASK to re-appraise.
* IMA_UPDATE_XATTR - indicates that security.ima needs to be updated.
  It is cleared if file policy changes and no update is needed.
* IMA_DIGSIG - indicates that file security.ima has signature and file
  security.ima must not update to file has on file close.
* IMA_MUST_MEASURE - indicates the file is in the measurement policy.

Changes in v6:
* introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in
  the measurement policy. It is used instead of the IMA_MEASURE (iint->flags)
  to detect ToMToU violation and when iint->mutex is unlocked and behind inode
  lock only (same as some other flags). Issue reported by Roberto Sassu.

Changes in v5:
* use of inode_lock() and inode_unlock()

Changes in v4:
* adoped to violation detection fixes
* added IMA_UPDATE_XATTR flag to require xattr update on file close

Changes in v3:
* prevent signature removal with new locking
* rename attr_flags to atomic_flags

Changes in v2:
* revert taking the i_mutex in integrity_inode_get() so that iint allocation
  could be done with i_mutex taken
* move taking the i_mutex from appraisal code to the process_measurement()

Signed-off-by: Dmitry Kasatkin 
---
 security/integrity/iint.c |  2 +
 security/integrity/ima/ima_appraise.c | 27 +++---
 security/integrity/ima/ima_main.c | 70 ---
 security/integrity/integrity.h| 18 ++---
 4 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c84e058..d726ba23 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -155,12 +155,14 @@ static void init_once(void *foo)
memset(iint, 0, sizeof(*iint));
iint->version = 0;
iint->flags = 0UL;
+   iint->atomic_flags = 0;
iint->ima_file_status = INTEGRITY_UNKNOWN;
iint->ima_mmap_status = INTEGRITY_UNKNOWN;
iint->ima_bprm_status = INTEGRITY_UNKNOWN;
iint->ima_read_status = INTEGRITY_UNKNOWN;
iint->evm_status = INTEGRITY_UNKNOWN;
iint->measured_pcrs = 0;
+   mutex_init(>mutex);
 }
 
 static int __init integrity_iintcache_init(void)
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index 9a54c77..3fc96dbd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_FAIL;
break;
}
+   clear_bit(IMA_DIGSIG, >atomic_flags);
if (xattr_len - sizeof(xattr_value->type) - hash_start >=
iint->ima_hash->length)
/* xattr length may be longer. md5 hash in previous
@@ -269,7 +270,7 @@ int