Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On 9 May 2014 23:07, J. R. Okajima wrote: > > Mimi Zohar: >> I assume so, as there wasn't any comment. As a temporary fix, would it >> make sense not to measure/appraise/audit files opened with the direct-io >> flag based policy? Define a new IMA policy option 'directio'. A sample >> rule would look like: >> >> dont_appraise bprm_check directio fsuuid=... > > I prefer such approach or anything addressing in IMA only, so it makes > sense. > > > J. R. Okajima We are working on i_mutex free solution. Let's hold on on this... -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On 9 May 2014 23:07, J. R. Okajima hooanon...@gmail.com wrote: Mimi Zohar: I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like: dont_appraise bprm_check directio fsuuid=... I prefer such approach or anything addressing in IMA only, so it makes sense. J. R. Okajima We are working on i_mutex free solution. Let's hold on on this... -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Mimi Zohar: > I assume so, as there wasn't any comment. As a temporary fix, would it > make sense not to measure/appraise/audit files opened with the direct-io > flag based policy? Define a new IMA policy option 'directio'. A sample > rule would look like: > > dont_appraise bprm_check directio fsuuid=... I prefer such approach or anything addressing in IMA only, so it makes sense. J. R. Okajima -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Sat, 2014-05-10 at 01:01 +0900, J. R. Okajima wrote: > Mimi Zohar: > > Another approach was posted here > > http://marc.info/?l=linux-security-module=138919062430367=2 which > > also was not upstreamed. > > It might be better a little than previous one which handles the flag > temporarily. But, in order to make the code cleaner particulary for > do_blockdev_direct_IO(), I'd suggest > - make two new static inline functions like > r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). > - these new functions are complied when CONFIG_IMA is enabled, otherwise > they are plain mutex_lock/unlock(). > - then do_blockdev_direct_IO() can call them blindly. > - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA > is enabled too. > > I can guess that several people thinks that is still "ugly locking", but > the deadlock is much ugly in real world. And we need some workaround for > it. I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like: dont_appraise bprm_check directio fsuuid=... Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, 2014-05-09 at 18:56 +0100, Al Viro wrote: > On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote: > > > > Dmitry Kasatkin: > > > Following patch replaces IMA usage of kernel_read() with special > > > version which skips security check that triggers kernel panic > > > when Apparmor and IMA appraisal are enabled together. > > > > I know this is related to exit(2), but this behaviour of IMA is related > > to open(2) too. > > When O_DIRECT is specified, some filesystems (for example, ext2) call > > do_blockdev_direct_IO() which acquires i_mutex. But > > IMA:process_measurement() already acquires i_mutex before kernel_read(). > > It causes a deadlock even if you replace kernel_read() by a simpler one. > > How can we stop reading the file from IMA? > > Disabling it would be a good start... And no, I'm not kidding - the mess > you are proposing is such that it's not at all obvious that this stuff > is worth the trouble. Al, perhaps we didn't do a good job describing the different use case scenarios for the different aspects of the integrity subsystem. Are you interested in hearing about them? > Why the devil is it playing with ->i_mutex? IMA, that is. Its use for > data is absolutely fs-dependent. Again, filesystem is *NOT* required > to take ->i_mutex anywhere on the write path. At all. Agreed it shouldn't be taking the i_mutex. However, there was a lock ordering issue when writing extended attributes, which does take the i_mutex. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote: > > Dmitry Kasatkin: > > Following patch replaces IMA usage of kernel_read() with special > > version which skips security check that triggers kernel panic > > when Apparmor and IMA appraisal are enabled together. > > I know this is related to exit(2), but this behaviour of IMA is related > to open(2) too. > When O_DIRECT is specified, some filesystems (for example, ext2) call > do_blockdev_direct_IO() which acquires i_mutex. But > IMA:process_measurement() already acquires i_mutex before kernel_read(). > It causes a deadlock even if you replace kernel_read() by a simpler one. > How can we stop reading the file from IMA? Disabling it would be a good start... And no, I'm not kidding - the mess you are proposing is such that it's not at all obvious that this stuff is worth the trouble. Why the devil is it playing with ->i_mutex? IMA, that is. Its use for data is absolutely fs-dependent. Again, filesystem is *NOT* required to take ->i_mutex anywhere on the write path. At all. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
"J. R. Okajima": > do_blockdev_direct_IO(), I'd suggest > - make two new static inline functions like > r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). > - these new functions are complied when CONFIG_IMA is enabled, otherwise > they are plain mutex_lock/unlock(). > - then do_blockdev_direct_IO() can call them blindly. > - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA > is enabled too. One more thing. Since struct file is a sharable object, it might be better to put task-id into struct file. Hmm, then should we support for multiple tasks by list or something? Oh the code grows... J. R. Okajima -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Mimi Zohar: > Another approach was posted here > http://marc.info/?l=linux-security-module=138919062430367=2 which > also was not upstreamed. It might be better a little than previous one which handles the flag temporarily. But, in order to make the code cleaner particulary for do_blockdev_direct_IO(), I'd suggest - make two new static inline functions like r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). - these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock(). - then do_blockdev_direct_IO() can call them blindly. - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too. I can guess that several people thinks that is still "ugly locking", but the deadlock is much ugly in real world. And we need some workaround for it. J. R. Okajima -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, 2014-05-09 at 18:17 +0900, J. R. Okajima wrote: > Dmitry Kasatkin: > > It is a different issue. > > > > I made patch more than a year ago which fix the problem > > > > https://lkml.org/lkml/2013/2/20/601 > > Yes, I know this is a different issue, but I didn't know the patch. > While I am not sure how ugly (or beautiful) is the approache the patch > took, as long as IMA needs to read the file, we should merge the merge. > Or we should wait for your another patch. Another approach was posted here http://marc.info/?l=linux-security-module=138919062430367=2 which also was not upstreamed. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Dmitry Kasatkin: > It is a different issue. > > I made patch more than a year ago which fix the problem > > https://lkml.org/lkml/2013/2/20/601 Yes, I know this is a different issue, but I didn't know the patch. While I am not sure how ugly (or beautiful) is the approache the patch took, as long as IMA needs to read the file, we should merge the merge. Or we should wait for your another patch. J. R. Okajima -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On 09/05/14 06:10, J. R. Okajima wrote: > Dmitry Kasatkin: >> Following patch replaces IMA usage of kernel_read() with special >> version which skips security check that triggers kernel panic >> when Apparmor and IMA appraisal are enabled together. > I know this is related to exit(2), but this behaviour of IMA is related > to open(2) too. > When O_DIRECT is specified, some filesystems (for example, ext2) call > do_blockdev_direct_IO() which acquires i_mutex. But > IMA:process_measurement() already acquires i_mutex before kernel_read(). > It causes a deadlock even if you replace kernel_read() by a simpler one. Hi, It is a different issue. I made patch more than a year ago which fix the problem https://lkml.org/lkml/2013/2/20/601 I think we had to declare the purpose of the patch in a bit different way. IMA really does not need direct-io, and can temporarily drop the flag. As side affect, it would fix the deadlock problem But I have a different patch now. I will post it today. > How can we stop reading the file from IMA? It is actually very interesting question... 1) if you would like to use IMA without it reading a file, then I think I must disappoint you. It is not possible.. IMA needs reading a file. 2) if you do not use IMA, then there is no problem for you, because IMA will not read file if it is not used... Have a nice day. - Dmitry > > J. R. Okajima > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On 09/05/14 06:10, J. R. Okajima wrote: Dmitry Kasatkin: Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together. I know this is related to exit(2), but this behaviour of IMA is related to open(2) too. When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one. Hi, It is a different issue. I made patch more than a year ago which fix the problem https://lkml.org/lkml/2013/2/20/601 I think we had to declare the purpose of the patch in a bit different way. IMA really does not need direct-io, and can temporarily drop the flag. As side affect, it would fix the deadlock problem But I have a different patch now. I will post it today. How can we stop reading the file from IMA? It is actually very interesting question... 1) if you would like to use IMA without it reading a file, then I think I must disappoint you. It is not possible.. IMA needs reading a file. 2) if you do not use IMA, then there is no problem for you, because IMA will not read file if it is not used... Have a nice day. - Dmitry J. R. Okajima -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Dmitry Kasatkin: It is a different issue. I made patch more than a year ago which fix the problem https://lkml.org/lkml/2013/2/20/601 Yes, I know this is a different issue, but I didn't know the patch. While I am not sure how ugly (or beautiful) is the approache the patch took, as long as IMA needs to read the file, we should merge the merge. Or we should wait for your another patch. J. R. Okajima -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, 2014-05-09 at 18:17 +0900, J. R. Okajima wrote: Dmitry Kasatkin: It is a different issue. I made patch more than a year ago which fix the problem https://lkml.org/lkml/2013/2/20/601 Yes, I know this is a different issue, but I didn't know the patch. While I am not sure how ugly (or beautiful) is the approache the patch took, as long as IMA needs to read the file, we should merge the merge. Or we should wait for your another patch. Another approach was posted here http://marc.info/?l=linux-security-modulem=138919062430367w=2 which also was not upstreamed. Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Mimi Zohar: Another approach was posted here http://marc.info/?l=linux-security-modulem=138919062430367w=2 which also was not upstreamed. It might be better a little than previous one which handles the flag temporarily. But, in order to make the code cleaner particulary for do_blockdev_direct_IO(), I'd suggest - make two new static inline functions like r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). - these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock(). - then do_blockdev_direct_IO() can call them blindly. - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too. I can guess that several people thinks that is still ugly locking, but the deadlock is much ugly in real world. And we need some workaround for it. J. R. Okajima -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
J. R. Okajima: do_blockdev_direct_IO(), I'd suggest - make two new static inline functions like r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). - these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock(). - then do_blockdev_direct_IO() can call them blindly. - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too. One more thing. Since struct file is a sharable object, it might be better to put task-id into struct file. Hmm, then should we support for multiple tasks by list or something? Oh the code grows... J. R. Okajima -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote: Dmitry Kasatkin: Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together. I know this is related to exit(2), but this behaviour of IMA is related to open(2) too. When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one. How can we stop reading the file from IMA? Disabling it would be a good start... And no, I'm not kidding - the mess you are proposing is such that it's not at all obvious that this stuff is worth the trouble. Why the devil is it playing with -i_mutex? IMA, that is. Its use for data is absolutely fs-dependent. Again, filesystem is *NOT* required to take -i_mutex anywhere on the write path. At all. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Fri, 2014-05-09 at 18:56 +0100, Al Viro wrote: On Fri, May 09, 2014 at 12:10:03PM +0900, J. R. Okajima wrote: Dmitry Kasatkin: Following patch replaces IMA usage of kernel_read() with special version which skips security check that triggers kernel panic when Apparmor and IMA appraisal are enabled together. I know this is related to exit(2), but this behaviour of IMA is related to open(2) too. When O_DIRECT is specified, some filesystems (for example, ext2) call do_blockdev_direct_IO() which acquires i_mutex. But IMA:process_measurement() already acquires i_mutex before kernel_read(). It causes a deadlock even if you replace kernel_read() by a simpler one. How can we stop reading the file from IMA? Disabling it would be a good start... And no, I'm not kidding - the mess you are proposing is such that it's not at all obvious that this stuff is worth the trouble. Al, perhaps we didn't do a good job describing the different use case scenarios for the different aspects of the integrity subsystem. Are you interested in hearing about them? Why the devil is it playing with -i_mutex? IMA, that is. Its use for data is absolutely fs-dependent. Again, filesystem is *NOT* required to take -i_mutex anywhere on the write path. At all. Agreed it shouldn't be taking the i_mutex. However, there was a lock ordering issue when writing extended attributes, which does take the i_mutex. Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
On Sat, 2014-05-10 at 01:01 +0900, J. R. Okajima wrote: Mimi Zohar: Another approach was posted here http://marc.info/?l=linux-security-modulem=138919062430367w=2 which also was not upstreamed. It might be better a little than previous one which handles the flag temporarily. But, in order to make the code cleaner particulary for do_blockdev_direct_IO(), I'd suggest - make two new static inline functions like r = ima_aware_file_inode_mutex_lock(file) and ..._unlock(r, file). - these new functions are complied when CONFIG_IMA is enabled, otherwise they are plain mutex_lock/unlock(). - then do_blockdev_direct_IO() can call them blindly. - of course, O_DIRECT_HAVELOCK should be complied only when CONFIG_IMA is enabled too. I can guess that several people thinks that is still ugly locking, but the deadlock is much ugly in real world. And we need some workaround for it. I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like: dont_appraise bprm_check directio fsuuid=... Mimi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: IMA + O_DIRECT (Re: [PATCH 0/1] fix IMA + Apparmor kernel panic)
Mimi Zohar: I assume so, as there wasn't any comment. As a temporary fix, would it make sense not to measure/appraise/audit files opened with the direct-io flag based policy? Define a new IMA policy option 'directio'. A sample rule would look like: dont_appraise bprm_check directio fsuuid=... I prefer such approach or anything addressing in IMA only, so it makes sense. J. R. Okajima -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
