Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 17:40, Dmitry Kasatkin wrote: On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry It seems you are right... That violation patch came in between locking patch was there. I do not remember why I have rebased it like it looks now. But it seems that violation checking needs to be moved under iint->mutex locking. Hmm. but why I have not done it like that 3 years ago :) I will think how to update it. Thanks for catching it up. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 17:40, Dmitry Kasatkin wrote: On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry It seems you are right... That violation patch came in between locking patch was there. I do not remember why I have rebased it like it looks now. But it seems that violation checking needs to be moved under iint->mutex locking. Hmm. but why I have not done it like that 3 years ago :) I will think how to update it. Thanks for catching it up. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Bo PENG, Qiuen PENG, Shengli WANG
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, 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. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi
[PATCHv5 1/1] ima: re-introduce own integrity cache lock
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. 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 | 65 --- security/integrity/integrity.h| 17 ++--- 4 files changed, 72 insertions(+), 39 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 ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; break; case EVM_IMA_XATTR_DIGSIG: - iint->flags |= IMA_DIGSIG; + set_bit(IMA_DIGSIG, >atomic_flags); rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, (const char
[PATCHv5 1/1] ima: re-introduce own integrity cache lock
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. 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 | 65 --- security/integrity/integrity.h| 17 ++--- 4 files changed, 72 insertions(+), 39 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 ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; break; case EVM_IMA_XATTR_DIGSIG: - iint->flags |= IMA_DIGSIG; + set_bit(IMA_DIGSIG, >atomic_flags); rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, (const char *)xattr_value, rc,