| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 Jun 2014 18:27:20 +0300
From: Dmitry Kasatkin <d.kasatkin@...sung.com>
To: viro@...iv.linux.org.uk, linux-security-module@...r.kernel.org,
eparis@...hat.com, zohar@...ux.vnet.ibm.com,
linux-ima-devel@...ts.sourceforge.net
Cc: linux-kernel@...r.kernel.org, jmorris@...ei.org,
dmitry.kasatkin@...il.com
Subject: Re: [PATCHv2 1/1] ima: re-introduce own integrity cache lock
Hi Mimi,
If there is no objections, should we queue this patch for next release?
- Dmitry
On 16/05/14 15:03, Dmitry Kasatkin wrote:
> 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.
>
> 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_mutex.
>
> Changes to v1:
> * 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 <d.kasatkin@...sung.com>
> ---
> security/integrity/iint.c | 2 ++
> security/integrity/ima/ima_appraise.c | 20 ++++++--------------
> security/integrity/ima/ima_main.c | 32 +++++++++++++++++++++-----------
> security/integrity/integrity.h | 5 +++++
> 4 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index a521edf..d293207 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -154,11 +154,13 @@ static void init_once(void *foo)
> memset(iint, 0, sizeof(*iint));
> iint->version = 0;
> iint->flags = 0UL;
> + iint->attr_flags = 0;
> iint->ima_file_status = INTEGRITY_UNKNOWN;
> iint->ima_mmap_status = INTEGRITY_UNKNOWN;
> iint->ima_bprm_status = INTEGRITY_UNKNOWN;
> iint->ima_module_status = INTEGRITY_UNKNOWN;
> iint->evm_status = INTEGRITY_UNKNOWN;
> + mutex_init(&iint->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 d3113d4..c49f8c3 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -289,7 +289,9 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
> if (rc < 0)
> return;
>
> + mutex_lock(&file_inode(file)->i_mutex);
> ima_fix_xattr(dentry, iint);
> + mutex_unlock(&file_inode(file)->i_mutex);
> }
>
> /**
> @@ -313,13 +315,8 @@ void ima_inode_post_setattr(struct dentry *dentry)
>
> must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
> iint = integrity_iint_find(inode);
> - if (iint) {
> - iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
> - IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
> - IMA_ACTION_FLAGS);
> - if (must_appraise)
> - iint->flags |= IMA_APPRAISE;
> - }
> + if (iint)
> + set_bit(IMA_CHANGE_ATTR, &iint->attr_flags);
> if (!must_appraise)
> rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
> return;
> @@ -349,13 +346,8 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
> return;
>
> iint = integrity_iint_find(inode);
> - if (!iint)
> - return;
> -
> - iint->flags &= ~IMA_DONE_MASK;
> - if (digsig)
> - iint->flags |= IMA_DIGSIG;
> - return;
> + if (iint)
> + set_bit(IMA_CHANGE_XATTR, &iint->attr_flags);
> }
>
> int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index bd7b4cb..fdd5320 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -88,8 +88,6 @@ static void ima_rdwr_violation_check(struct file *file)
> if (!S_ISREG(inode->i_mode) || !ima_initialized)
> return;
>
> - mutex_lock(&inode->i_mutex); /* file metadata: permissions, xattr */
> -
> if (mode & FMODE_WRITE) {
> if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
> struct integrity_iint_cache *iint;
> @@ -104,8 +102,6 @@ static void ima_rdwr_violation_check(struct file *file)
> send_writers = true;
> }
>
> - mutex_unlock(&inode->i_mutex);
> -
> if (!send_tomtou && !send_writers)
> return;
>
> @@ -127,14 +123,14 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> if (!(mode & FMODE_WRITE))
> return;
>
> - mutex_lock(&inode->i_mutex);
> + mutex_lock(&iint->mutex);
> if (atomic_read(&inode->i_writecount) == 1 &&
> iint->version != inode->i_version) {
> iint->flags &= ~IMA_DONE_MASK;
> if (iint->flags & IMA_APPRAISE)
> ima_update_xattr(iint, file);
> }
> - mutex_unlock(&inode->i_mutex);
> + mutex_unlock(&iint->mutex);
> }
>
> /**
> @@ -187,10 +183,21 @@ static int process_measurement(struct file *file, const char *filename,
> _func = (action & IMA_FILE_APPRAISE) ? FILE_CHECK : function;
>
> mutex_lock(&inode->i_mutex);
> -
> iint = integrity_inode_get(inode);
> + mutex_unlock(&inode->i_mutex);
> if (!iint)
> - goto out;
> + goto out_unlocked;
> +
> + mutex_lock(&iint->mutex);
> +
> + if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->attr_flags))
> + /* reset appraisal flags if ima_inode_post_setattr was called */
> + iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
> + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK);
> +
> + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->attr_flags))
> + /* reset all flags if ima_inode_setxattr was called */
> + iint->flags &= ~IMA_DONE_MASK;
>
> /* Determine if already appraised/measured based on bitmask
> * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
> @@ -225,18 +232,21 @@ static int process_measurement(struct file *file, const char *filename,
> if (action & IMA_MEASURE)
> ima_store_measurement(iint, file, pathname,
> xattr_value, xattr_len);
> - if (action & IMA_APPRAISE_SUBMASK)
> + if (action & IMA_APPRAISE_SUBMASK) {
> + mutex_lock(&inode->i_mutex);
> rc = ima_appraise_measurement(_func, iint, file, pathname,
> xattr_value, xattr_len);
> + mutex_unlock(&inode->i_mutex);
> + }
> if (action & IMA_AUDIT)
> ima_audit_measurement(iint, pathname);
> kfree(pathbuf);
> out_digsig:
> if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
> rc = -EACCES;
> -out:
> - mutex_unlock(&inode->i_mutex);
> + mutex_unlock(&iint->mutex);
> kfree(xattr_value);
> +out_unlocked:
> if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
> return -EACCES;
> return 0;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 2fb5e53..f73cd06 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -50,6 +50,9 @@
> #define IMA_APPRAISED_SUBMASK (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
> IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED)
>
> +#define IMA_CHANGE_XATTR 0
> +#define IMA_CHANGE_ATTR 1
> +
> enum evm_ima_xattr_type {
> IMA_XATTR_DIGEST = 0x01,
> EVM_XATTR_HMAC,
> @@ -96,9 +99,11 @@ struct signature_v2_hdr {
> /* integrity data associated with an inode */
> struct integrity_iint_cache {
> struct rb_node rb_node; /* rooted in integrity_iint_tree */
> + struct mutex mutex; /* protects: version, flags, digest */
> struct inode *inode; /* back pointer to inode in question */
> u64 version; /* track inode changes */
> unsigned long flags;
> + unsigned long attr_flags;
> enum integrity_status ima_file_status:4;
> enum integrity_status ima_mmap_status:4;
> enum integrity_status ima_bprm_status:4;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists