ima: fix race condition on ima_rdwr_violation_check and process_measurement

This patch fixes a race condition between two functions that try to access
the same inode. Since the i_mutex lock is held and released separately
in the two functions, there may be the possibility that a violation is
not correctly detected.

Suppose there are two processes, A (reader) and B (writer), if the
following sequence happens:

A: ima_rdwr_violation_check()
B: ima_rdwr_violation_check()
B: process_measurement()
B: starts writing the inode
A: process_measurement()

the ToMToU violation (a reader may be accessing a content different from
that measured, due to a concurrent modification by a writer) will not be
detected. To avoid this issue, the violation check and the measurement
must be done atomically.

This patch fixes the problem by moving the violation check inside
process_measurement() when the i_mutex lock is held. Differently from
the old code, the violation check is executed also for the MMAP_CHECK
hook (other than for FILE_CHECK). This allows to detect ToMToU violations
that are possible because shared libraries can be opened for writing
while they are in use (according to the output of 'man mmap', the mmap()
flag MAP_DENYWRITE is ignored).

Changes in v5 (Roberto Sassu):
* get iint if action is not zero
* exit process_measurement() after the violation check if action is zero
* reverse order process_measurement() exit cleanup (Mimi)

Changes in v4 (Dmitry Kasatkin):
* iint allocation is done before calling ima_rdrw_violation_check()
  (Suggested-by Mimi)
* do not check for violations if the policy does not contain 'measure'
  rules (done by Roberto Sassu)

Changes in v3 (Dmitry Kasatkin):
* no violation checking for MMAP_CHECK function in this patch
* remove use of filename from violation
* removes checking if ima is enabled from ima_rdrw_violation_check
* slight style change

Suggested-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

1 files changed, 33 insertions, 21 deletions

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2191b36ad1da..03bb52ecf490 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -77,21 +77,19 @@ __setup("ima_hash=", hash_setup);
  *        could result in a file measurement error.
  *
  */
-static void ima_rdwr_violation_check(struct file *file)
+static void ima_rdwr_violation_check(struct file *file,
+                                     struct integrity_iint_cache *iint,
+                                     char **pathbuf,
+                                     const char **pathname)
 {
         struct inode *inode = file_inode(file);
         fmode_t mode = file->f_mode;
         bool send_tomtou = false, send_writers = false;
-        char *pathbuf = NULL;
-        const char *pathname;
-
-        if (!S_ISREG(inode->i_mode) || !(ima_policy_flag & IMA_MEASURE))
-                return;
 
         if (mode & FMODE_WRITE) {
                 if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
-                        struct integrity_iint_cache *iint;
-                        iint = integrity_iint_find(inode);
+                        if (!iint)
+                                iint = integrity_iint_find(inode);
                         /* IMA_MEASURE is set from reader side */
                         if (iint && (iint->flags & IMA_MEASURE))
                                 send_tomtou = true;
@@ -105,14 +103,13 @@ static void ima_rdwr_violation_check(struct file *file)
         if (!send_tomtou && !send_writers)
                 return;
 
-        pathname = ima_d_path(&file->f_path, &pathbuf);
+        *pathname = ima_d_path(&file->f_path, pathbuf);
 
         if (send_tomtou)
-                ima_add_violation(file, pathname, "invalid_pcr", "ToMToU");
+                ima_add_violation(file, *pathname, "invalid_pcr", "ToMToU");
         if (send_writers)
-                ima_add_violation(file, pathname,
+                ima_add_violation(file, *pathname,
                                   "invalid_pcr", "open_writers");
-        kfree(pathbuf);
 }
 
 static void ima_check_last_writer(struct integrity_iint_cache *iint,
@@ -160,13 +157,14 @@ static int process_measurement(struct file *file, int mask, int function,
                                int opened)
 {
         struct inode *inode = file_inode(file);
-        struct integrity_iint_cache *iint;
+        struct integrity_iint_cache *iint = NULL;
         struct ima_template_desc *template_desc;
         char *pathbuf = NULL;
         const char *pathname = NULL;
         int rc = -ENOMEM, action, must_appraise;
         struct evm_ima_xattr_data *xattr_value = NULL, **xattr_ptr = NULL;
         int xattr_len = 0;
+        bool violation_check;
 
         if (!ima_policy_flag || !S_ISREG(inode->i_mode))
                 return 0;
@@ -176,7 +174,9 @@ static int process_measurement(struct file *file, int mask, int function,
          * Included is the appraise submask.
          */
         action = ima_get_action(inode, mask, function);
-        if (!action)
+        violation_check = (function == FILE_CHECK &&
+                           (ima_policy_flag & IMA_MEASURE));
+        if (!action && !violation_check)
                 return 0;
 
         must_appraise = action & IMA_APPRAISE;
@@ -187,9 +187,19 @@ static int process_measurement(struct file *file, int mask, int function,
 
         mutex_lock(&inode->i_mutex);
 
-        iint = integrity_inode_get(inode);
-        if (!iint)
-                goto out;
+        if (action) {
+                iint = integrity_inode_get(inode);
+                if (!iint)
+                        goto out;
+        }
+
+        if (violation_check) {
+                ima_rdwr_violation_check(file, iint, &pathbuf, &pathname);
+                if (!action) {
+                        rc = 0;
+                        goto out_free;
+                }
+        }
 
         /* Determine if already appraised/measured based on bitmask
          * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
@@ -218,7 +228,8 @@ static int process_measurement(struct file *file, int mask, int function,
                 goto out_digsig;
         }
 
-        pathname = ima_d_path(&file->f_path, &pathbuf);
+        if (!pathname)  /* ima_rdwr_violation possibly pre-fetched */
+                pathname = ima_d_path(&file->f_path, &pathbuf);
 
         if (action & IMA_MEASURE)
                 ima_store_measurement(iint, file, pathname,
@@ -228,13 +239,15 @@ static int process_measurement(struct file *file, int mask, int function,
                                               xattr_value, xattr_len, opened);
         if (action & IMA_AUDIT)
                 ima_audit_measurement(iint, pathname);
-        kfree(pathbuf);
+
 out_digsig:
         if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
                 rc = -EACCES;
+        kfree(xattr_value);
+out_free:
+        kfree(pathbuf);
 out:
         mutex_unlock(&inode->i_mutex);
-        kfree(xattr_value);
         if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
                 return -EACCES;
         return 0;
@@ -288,7 +301,6 @@ int ima_bprm_check(struct linux_binprm *bprm)
  */
 int ima_file_check(struct file *file, int mask, int opened)
 {
-        ima_rdwr_violation_check(file);
         return process_measurement(file,
                                    mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
                                    FILE_CHECK, opened);