Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
From: Tyler Hicks <tyhicks@linux.microsoft.com>
To: Mimi Zohar <zohar@linux.ibm.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: James Morris <jmorris@namei.org>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
	Prakhar Srivastava <prsriva02@gmail.com>,
	linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: [PATCH v2 02/11] ima: Free the entire rule when deleting a list of rules
Date: Fri, 26 Jun 2020 17:38:51 -0500
Message-ID: <20200626223900.253615-3-tyhicks@linux.microsoft.com> (raw)
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>

Create a function, ima_free_rule(), to free all memory associated with
an ima_rule_entry. Use the new function to fix memory leaks of allocated
ima_rule_entry members, such as .fsname and .keyrings, when deleting a
list of rules.

Make the existing ima_lsm_free_rule() function specific to the LSM
audit rule array of an ima_rule_entry and require that callers make an
additional call to kfree to free the ima_rule_entry itself.

This fixes a memory leak seen when loading by a valid rule that contains
an additional piece of allocated memory, such as an fsname, followed by
an invalid rule that triggers a policy load failure:

 # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
    /sys/kernel/security/ima/policy
 -bash: echo: write error: Invalid argument
 # echo scan > /sys/kernel/debug/kmemleak
 # cat /sys/kernel/debug/kmemleak
 unreferenced object 0xffff9bab67ca12c0 (size 16):
   comm "bash", pid 684, jiffies 4295212803 (age 252.344s)
   hex dump (first 16 bytes):
     73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5  securityfs.kkkk.
   backtrace:
     [<00000000adc80b1b>] kstrdup+0x2e/0x60
     [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
     [<00000000444825ac>] ima_write_policy+0xab/0x1d0
     [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
     [<0000000096feedcf>] ksys_write+0x68/0xe0
     [<0000000052b544a2>] do_syscall_64+0x56/0xa0
     [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v2
  - Collapsed patch #2 from v1 of this series, into this patch. This
    patch now introduces ima_free_rule().
  - Existing callers of ima_lsm_free_rule() are doing so to free rules
    after a successful or failed ima_lsm_copy_rule() and those callers
    continue to directly call ima_lsm_copy_rule() rather than doing
    explicit reference ownership and calling ima_free_rule().
  - The kfree(entry) of ima_lsm_free_rule() was removed from that
    function to make it focused on freeing the LSM references. Direct
    callers of ima_lsm_free_rule() must now call kfree(entry) after
    ima_lsm_free_rule().
  - A comment was added in ima_lsm_update_rule() to clarify why
    ima_free_rule() isn't being used.

 security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d7c268c2b0ce..bf00b966e87f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -261,6 +261,21 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 		security_filter_rule_free(entry->lsm[i].rule);
 		kfree(entry->lsm[i].args_p);
 	}
+}
+
+static void ima_free_rule(struct ima_rule_entry *entry)
+{
+	if (!entry)
+		return;
+
+	/*
+	 * entry->template->fields may be allocated in ima_parse_rule() but that
+	 * reference is owned by the corresponding ima_template_desc element in
+	 * the defined_templates list and cannot be freed here
+	 */
+	kfree(entry->fsname);
+	kfree(entry->keyrings);
+	ima_lsm_free_rule(entry);
 	kfree(entry);
 }
 
@@ -302,6 +317,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 
 out_err:
 	ima_lsm_free_rule(nentry);
+	kfree(nentry);
 	return NULL;
 }
 
@@ -315,7 +331,14 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
 
 	list_replace_rcu(&entry->list, &nentry->list);
 	synchronize_rcu();
+	/*
+	 * ima_lsm_copy_rule() shallow copied all references, except for the
+	 * LSM references, from entry to nentry so we only want to free the LSM
+	 * references and the entry itself. All other memory refrences will now
+	 * be owned by nentry.
+	 */
 	ima_lsm_free_rule(entry);
+	kfree(entry);
 
 	return 0;
 }
@@ -1402,15 +1425,11 @@ ssize_t ima_parse_add_rule(char *rule)
 void ima_delete_rules(void)
 {
 	struct ima_rule_entry *entry, *tmp;
-	int i;
 
 	temp_ima_appraise = 0;
 	list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
-		for (i = 0; i < MAX_LSM_RULES; i++)
-			kfree(entry->lsm[i].args_p);
-
 		list_del(&entry->list);
-		kfree(entry);
+		ima_free_rule(entry);
 	}
 }
 
-- 
2.25.1


  parent reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-26 22:38 [PATCH v2 00/11] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
2020-06-26 22:38 ` [PATCH v2 01/11] ima: Have the LSM free its audit rule Tyler Hicks
2020-06-26 22:38 ` Tyler Hicks [this message]
2020-06-26 22:38 ` [PATCH v2 03/11] ima: Free the entire rule if it fails to parse Tyler Hicks
2020-06-26 22:38 ` [PATCH v2 04/11] ima: Fail rule parsing when buffer hook functions have an invalid action Tyler Hicks
2020-06-26 22:38 ` [PATCH v2 05/11] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond Tyler Hicks
2020-06-27 23:40   ` Lakshmi Ramasubramanian
2020-06-26 22:38 ` [PATCH v2 06/11] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
2020-06-27 23:39   ` Lakshmi Ramasubramanian
2020-06-26 22:38 ` [PATCH v2 07/11] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
2020-06-26 22:38 ` [PATCH v2 08/11] ima: Use correct type for " Tyler Hicks
2020-06-26 22:38 ` [PATCH v2 09/11] ima: Move validation of the keyrings conditional into ima_validate_rule() Tyler Hicks
2020-06-27 23:49   ` Lakshmi Ramasubramanian
2020-06-29 14:16     ` Tyler Hicks
2020-06-30 23:07   ` Mimi Zohar
2020-07-02 22:16     ` Tyler Hicks
2020-07-03 14:15       ` Mimi Zohar
2020-07-06 13:18         ` Tyler Hicks
2020-07-07  3:18           ` Mimi Zohar
2020-06-26 22:38 ` [PATCH v2 10/11] ima: Use the common function to detect LSM conditionals in a rule Tyler Hicks
2020-06-26 22:39 ` [PATCH v2 11/11] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Tyler Hicks
2020-06-28  0:03   ` Lakshmi Ramasubramanian
2020-07-01  8:04   ` Dave Young
2020-07-01 14:38     ` Tyler Hicks
2020-07-01  0:29 ` [PATCH v2 00/11] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Mimi Zohar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200626223900.253615-3-tyhicks@linux.microsoft.com \
    --to=tyhicks@linux.microsoft.com \
    --cc=dmitry.kasatkin@gmail.com \
    --cc=jmorris@namei.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=nramas@linux.microsoft.com \
    --cc=prsriva02@gmail.com \
    --cc=serge@hallyn.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git