From patchwork Fri Dec 1 18:40:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Kasatkin X-Patchwork-Id: 10087747 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B81DC6037E for ; Fri, 1 Dec 2017 18:40:18 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AA9982A1B0 for ; Fri, 1 Dec 2017 18:40:18 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9F2502A579; Fri, 1 Dec 2017 18:40:18 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 905C92A55F for ; Fri, 1 Dec 2017 18:40:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751806AbdLASkQ (ORCPT ); Fri, 1 Dec 2017 13:40:16 -0500 Received: from mail-lf0-f68.google.com ([209.85.215.68]:37058 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751369AbdLASkP (ORCPT ); Fri, 1 Dec 2017 13:40:15 -0500 Received: by mail-lf0-f68.google.com with SMTP id a12so12739633lfe.4; Fri, 01 Dec 2017 10:40:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=QH5tytuhC7UOxrtwuB6wtffvVl5Y2zv5tKXXZ/VF3J4=; b=nuNRM8tAMkGVgNTkNNolSpSk5DDdb3eaPZhdoZhXrbYKbGBznuljhXnM17mwO5Lh+f Oat4wOa68owhQvMoceHV2cIZF0twrFZdW1WMgi81x7qjm5md1kz0DVrXnY9qpfOmQUhO wiVSRBJaWc2esRoUBTZMZy7dL026mN4TFCf/apgs57ZV/+NU8r9xaFJFZNkXswqtgNSg MI4U9do/xTUjG/4u+o9usV0vRjqiZKGAN2zAWmb8Vxxy6GMsPBuvULsxECIagsWkxst2 jyYRrjbicoe+2layhhaew265Guguq40+Teiv8rD87ZFXwYFtxxrxOSEQLhQzjgPQrUGc b/BQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=QH5tytuhC7UOxrtwuB6wtffvVl5Y2zv5tKXXZ/VF3J4=; b=K90pgLtEHUtZtbC8siyGQd0bYvu4zELKdfUAgwOkGfxYhryc7Xb0DRdRE410ffigbw 12NJ9HEg+ek5JTe/k4FkB20v9AJ95rfaX/WFl/yhGILFUfjW26iNQ9FvXDyyElS85rPh fj/udTWAmfx8GWjwDHJSDTfMxZZNjITY0rOXyzNxezEHiXtUhsuO53pupl9GhVCYVnSO z25bvGK6wzaeQPbuVcuhtNdKkqiqWMSM6MHsp8elivssmnGn3S4W/s895/G6PqAyNVU3 HD2UCQkwOn83RrIAkWEuiju9k1xo5id+zx3enU+o6tFnhCyJwri/8njLKDGM/FQgBFVV xVHw== X-Gm-Message-State: AJaThX50i2mstingsR5DBPv0P3oDbUcesk9LKXWG7CD7QESqbcbqHHe5 Ycoc6RrSwe+5Jy2+9TN2MVg= X-Google-Smtp-Source: AGs4zMa2sOblb49F2sKFErPNmEmQcEkgmk3yZyTGk3kmEIvTGJPHtXyjwwNDDHxHtWkjDkYdYLpJdw== X-Received: by 10.46.84.65 with SMTP id y1mr5323100ljd.74.1512153612708; Fri, 01 Dec 2017 10:40:12 -0800 (PST) Received: from localhost.localdomain (a91-155-126-51.elisa-laajakaista.fi. [91.155.126.51]) by smtp.googlemail.com with ESMTPSA id n9sm1432267ljb.1.2017.12.01.10.40.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 01 Dec 2017 10:40:12 -0800 (PST) From: Dmitry Kasatkin X-Google-Original-From: Dmitry Kasatkin To: zohar@linux.vnet.ibm.com, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org Cc: linux-kernel@vger.kernel.org, hch@infradead.org, linux-fsdevel@vger.kernel.org, Dmitry Kasatkin Subject: [PATCHv5 1/1] ima: re-introduce own integrity cache lock Date: Fri, 1 Dec 2017 20:40:09 +0200 Message-Id: X-Mailer: git-send-email 2.7.4 Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP 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(&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 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, &iint->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, &iint->atomic_flags); rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, (const char *)xattr_value, rc, iint->ima_hash->digest, @@ -320,14 +321,16 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file) int rc = 0; /* do not collect and update hash for digital signatures */ - if (iint->flags & IMA_DIGSIG) + if (test_bit(IMA_DIGSIG, &iint->atomic_flags)) return; rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo); if (rc < 0) return; + inode_lock(file_inode(file)); ima_fix_xattr(dentry, iint); + inode_unlock(file_inode(file)); } /** @@ -350,16 +353,14 @@ void ima_inode_post_setattr(struct dentry *dentry) return; must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR); + if (!must_appraise) + __vfs_removexattr(dentry, XATTR_NAME_IMA); iint = integrity_iint_find(inode); if (iint) { - iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED | - IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | - IMA_ACTION_RULE_FLAGS); - if (must_appraise) - iint->flags |= IMA_APPRAISE; + set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags); + if (!must_appraise) + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); } - if (!must_appraise) - __vfs_removexattr(dentry, XATTR_NAME_IMA); } /* @@ -388,12 +389,12 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) iint = integrity_iint_find(inode); if (!iint) return; - - iint->flags &= ~IMA_DONE_MASK; iint->measured_pcrs = 0; + set_bit(IMA_CHANGE_XATTR, &iint->atomic_flags); if (digsig) - iint->flags |= IMA_DIGSIG; - return; + set_bit(IMA_DIGSIG, &iint->atomic_flags); + else + clear_bit(IMA_DIGSIG, &iint->atomic_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 7706546..69674bc 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -121,21 +121,24 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, struct inode *inode, struct file *file) { fmode_t mode = file->f_mode; + bool update; if (!(mode & FMODE_WRITE)) return; - inode_lock(inode); + mutex_lock(&iint->mutex); if (atomic_read(&inode->i_writecount) == 1) { + update = test_and_clear_bit(IMA_UPDATE_XATTR, + &iint->atomic_flags); if ((iint->version != inode->i_version) || (iint->flags & IMA_NEW_FILE)) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); iint->measured_pcrs = 0; - if (iint->flags & IMA_APPRAISE) + if (update) ima_update_xattr(iint, file); } } - inode_unlock(inode); + mutex_unlock(&iint->mutex); } /** @@ -168,7 +171,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, char *pathbuf = NULL; char filename[NAME_MAX]; const char *pathname = NULL; - int rc = -ENOMEM, action, must_appraise; + int rc = 0, action, must_appraise = 0; int pcr = CONFIG_IMA_MEASURE_PCR_IDX; struct evm_ima_xattr_data *xattr_value = NULL; int xattr_len = 0; @@ -199,17 +202,31 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if (action) { iint = integrity_inode_get(inode); if (!iint) - goto out; + rc = -ENOMEM; } - if (violation_check) { + if (!rc && violation_check) ima_rdwr_violation_check(file, iint, action & IMA_MEASURE, &pathbuf, &pathname); - if (!action) { - rc = 0; - goto out_free; - } - } + + inode_unlock(inode); + + if (rc) + goto out; + if (!action) + goto out; + + mutex_lock(&iint->mutex); + + if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags)) + /* reset appraisal flags if ima_inode_post_setattr was called */ + iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED | + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | + IMA_ACTION_FLAGS); + + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_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, @@ -227,7 +244,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if (!action) { if (must_appraise) rc = ima_get_cache_status(iint, func); - goto out_digsig; + goto out_locked; } template_desc = ima_template_desc_current(); @@ -240,7 +257,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, rc = ima_collect_measurement(iint, file, buf, size, hash_algo); if (rc != 0 && rc != -EBADF && rc != -EINVAL) - goto out_digsig; + goto out_locked; if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ pathname = ima_d_path(&file->f_path, &pathbuf, filename); @@ -248,26 +265,32 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if (action & IMA_MEASURE) ima_store_measurement(iint, file, pathname, xattr_value, xattr_len, pcr); - if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) + if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) { + inode_lock(inode); rc = ima_appraise_measurement(func, iint, file, pathname, xattr_value, xattr_len, opened); + inode_unlock(inode); + } if (action & IMA_AUDIT) ima_audit_measurement(iint, pathname); if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) rc = 0; -out_digsig: - if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && +out_locked: + if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) && !(iint->flags & IMA_NEW_FILE)) rc = -EACCES; + mutex_unlock(&iint->mutex); kfree(xattr_value); -out_free: +out: if (pathbuf) __putname(pathbuf); -out: - inode_unlock(inode); - if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) - return -EACCES; + if (must_appraise) { + if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE)) + return -EACCES; + if (file->f_mode & FMODE_WRITE) + set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); + } return 0; } diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index e324bf9..c053a14 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -29,11 +29,10 @@ /* iint cache flags */ #define IMA_ACTION_FLAGS 0xff000000 #define IMA_ACTION_RULE_FLAGS 0x06000000 -#define IMA_DIGSIG 0x01000000 -#define IMA_DIGSIG_REQUIRED 0x02000000 -#define IMA_PERMIT_DIRECTIO 0x04000000 -#define IMA_NEW_FILE 0x08000000 -#define EVM_IMMUTABLE_DIGSIG 0x10000000 +#define IMA_DIGSIG_REQUIRED 0x01000000 +#define IMA_PERMIT_DIRECTIO 0x02000000 +#define IMA_NEW_FILE 0x04000000 +#define EVM_IMMUTABLE_DIGSIG 0x08000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_APPRAISE_SUBMASK) @@ -54,6 +53,12 @@ #define IMA_APPRAISED_SUBMASK (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \ IMA_BPRM_APPRAISED | IMA_READ_APPRAISED) +/* iint cache atomic_flags */ +#define IMA_CHANGE_XATTR 0 +#define IMA_UPDATE_XATTR 1 +#define IMA_CHANGE_ATTR 2 +#define IMA_DIGSIG 3 + enum evm_ima_xattr_type { IMA_XATTR_DIGEST = 0x01, EVM_XATTR_HMAC, @@ -102,10 +107,12 @@ 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 measured_pcrs; + unsigned long atomic_flags; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; enum integrity_status ima_bprm_status:4;