All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
To: zohar@linux.ibm.com, James.Bottomley@HansenPartnership.com,
	linux-integrity@vger.kernel.org
Cc: dhowells@redhat.com, sashal@kernel.org,
	linux-kernel@vger.kernel.org, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org
Subject: [PATCH v1] IMA: pre-allocate buffer to hold keyrings string
Date: Thu, 16 Jan 2020 15:46:23 -0800	[thread overview]
Message-ID: <20200116234623.2959-1-nramas@linux.microsoft.com> (raw)

ima_match_keyring() is called while holding rcu read lock. Since this
function executes in atmomic context, it should not call any function
that can sleep (such as kstrdup()).

This patch pre-allocates a buffer to hold the keyrings string read from
the IMA policy and uses that to match the given keyring.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Fixes: e9085e0ad38a ("IMA: Add support to limit measuring keys")
---
 security/integrity/ima/ima_policy.c | 50 ++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9963863d6c92..180e2069e075 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -208,6 +208,10 @@ static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
 static struct list_head *ima_rules;
 
+/* Pre-allocated buffer used for matching keyrings. */
+static char *ima_keyrings;
+static size_t ima_keyrings_len;
+
 static int ima_policy __initdata;
 
 static int __init default_measure_policy_setup(char *str)
@@ -369,7 +373,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 static bool ima_match_keyring(struct ima_rule_entry *rule,
 			      const char *keyring, const struct cred *cred)
 {
-	char *keyrings, *next_keyring, *keyrings_ptr;
+	char *next_keyring, *keyrings_ptr;
 	bool matched = false;
 
 	if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
@@ -381,15 +385,13 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
 	if (!keyring)
 		return false;
 
-	keyrings = kstrdup(rule->keyrings, GFP_KERNEL);
-	if (!keyrings)
-		return false;
+	strcpy(ima_keyrings, rule->keyrings);
 
 	/*
 	 * "keyrings=" is specified in the policy in the format below:
 	 * keyrings=.builtin_trusted_keys|.ima|.evm
 	 */
-	keyrings_ptr = keyrings;
+	keyrings_ptr = ima_keyrings;
 	while ((next_keyring = strsep(&keyrings_ptr, "|")) != NULL) {
 		if (!strcmp(next_keyring, keyring)) {
 			matched = true;
@@ -397,8 +399,6 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
 		}
 	}
 
-	kfree(keyrings);
-
 	return matched;
 }
 
@@ -949,6 +949,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	bool uid_token;
 	struct ima_template_desc *template_desc;
 	int result = 0;
+	size_t keyrings_len;
 
 	ab = integrity_audit_log_start(audit_context(), GFP_KERNEL,
 				       AUDIT_INTEGRITY_POLICY_RULE);
@@ -1114,14 +1115,47 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 		case Opt_keyrings:
 			ima_log_string(ab, "keyrings", args[0].from);
 
+			keyrings_len = strlen(args[0].from) + 1;
+
 			if ((entry->keyrings) ||
 			    (entry->action != MEASURE) ||
-			    (entry->func != KEY_CHECK)) {
+			    (entry->func != KEY_CHECK) ||
+			    (keyrings_len < 2)) {
 				result = -EINVAL;
 				break;
 			}
+
+			if (ima_keyrings) {
+				if (keyrings_len > ima_keyrings_len) {
+					char *tmpbuf;
+
+					tmpbuf = krealloc(ima_keyrings,
+							  keyrings_len,
+							  GFP_KERNEL);
+					if (!tmpbuf) {
+						result = -ENOMEM;
+						break;
+					}
+
+					ima_keyrings = tmpbuf;
+					ima_keyrings_len = keyrings_len;
+				}
+			} else {
+				ima_keyrings = kzalloc(keyrings_len,
+						       GFP_KERNEL);
+				if (!ima_keyrings) {
+					result = -ENOMEM;
+					break;
+				}
+
+				ima_keyrings_len = keyrings_len;
+			}
+
 			entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
 			if (!entry->keyrings) {
+				kfree(ima_keyrings);
+				ima_keyrings = NULL;
+				ima_keyrings_len = 0;
 				result = -ENOMEM;
 				break;
 			}
-- 
2.17.1


WARNING: multiple messages have this Message-ID (diff)
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
To: zohar@linux.ibm.com, James.Bottomley@HansenPartnership.com,
	linux-integrity@vger.kernel.org
Cc: dhowells@redhat.com, sashal@kernel.org,
	linux-kernel@vger.kernel.org, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org
Subject: [PATCH v1] IMA: pre-allocate buffer to hold keyrings string
Date: Thu, 16 Jan 2020 23:46:23 +0000	[thread overview]
Message-ID: <20200116234623.2959-1-nramas@linux.microsoft.com> (raw)

ima_match_keyring() is called while holding rcu read lock. Since this
function executes in atmomic context, it should not call any function
that can sleep (such as kstrdup()).

This patch pre-allocates a buffer to hold the keyrings string read from
the IMA policy and uses that to match the given keyring.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Fixes: e9085e0ad38a ("IMA: Add support to limit measuring keys")
---
 security/integrity/ima/ima_policy.c | 50 ++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9963863d6c92..180e2069e075 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -208,6 +208,10 @@ static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
 static struct list_head *ima_rules;
 
+/* Pre-allocated buffer used for matching keyrings. */
+static char *ima_keyrings;
+static size_t ima_keyrings_len;
+
 static int ima_policy __initdata;
 
 static int __init default_measure_policy_setup(char *str)
@@ -369,7 +373,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 static bool ima_match_keyring(struct ima_rule_entry *rule,
 			      const char *keyring, const struct cred *cred)
 {
-	char *keyrings, *next_keyring, *keyrings_ptr;
+	char *next_keyring, *keyrings_ptr;
 	bool matched = false;
 
 	if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
@@ -381,15 +385,13 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
 	if (!keyring)
 		return false;
 
-	keyrings = kstrdup(rule->keyrings, GFP_KERNEL);
-	if (!keyrings)
-		return false;
+	strcpy(ima_keyrings, rule->keyrings);
 
 	/*
 	 * "keyrings=" is specified in the policy in the format below:
 	 * keyrings=.builtin_trusted_keys|.ima|.evm
 	 */
-	keyrings_ptr = keyrings;
+	keyrings_ptr = ima_keyrings;
 	while ((next_keyring = strsep(&keyrings_ptr, "|")) != NULL) {
 		if (!strcmp(next_keyring, keyring)) {
 			matched = true;
@@ -397,8 +399,6 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
 		}
 	}
 
-	kfree(keyrings);
-
 	return matched;
 }
 
@@ -949,6 +949,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	bool uid_token;
 	struct ima_template_desc *template_desc;
 	int result = 0;
+	size_t keyrings_len;
 
 	ab = integrity_audit_log_start(audit_context(), GFP_KERNEL,
 				       AUDIT_INTEGRITY_POLICY_RULE);
@@ -1114,14 +1115,47 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 		case Opt_keyrings:
 			ima_log_string(ab, "keyrings", args[0].from);
 
+			keyrings_len = strlen(args[0].from) + 1;
+
 			if ((entry->keyrings) ||
 			    (entry->action != MEASURE) ||
-			    (entry->func != KEY_CHECK)) {
+			    (entry->func != KEY_CHECK) ||
+			    (keyrings_len < 2)) {
 				result = -EINVAL;
 				break;
 			}
+
+			if (ima_keyrings) {
+				if (keyrings_len > ima_keyrings_len) {
+					char *tmpbuf;
+
+					tmpbuf = krealloc(ima_keyrings,
+							  keyrings_len,
+							  GFP_KERNEL);
+					if (!tmpbuf) {
+						result = -ENOMEM;
+						break;
+					}
+
+					ima_keyrings = tmpbuf;
+					ima_keyrings_len = keyrings_len;
+				}
+			} else {
+				ima_keyrings = kzalloc(keyrings_len,
+						       GFP_KERNEL);
+				if (!ima_keyrings) {
+					result = -ENOMEM;
+					break;
+				}
+
+				ima_keyrings_len = keyrings_len;
+			}
+
 			entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
 			if (!entry->keyrings) {
+				kfree(ima_keyrings);
+				ima_keyrings = NULL;
+				ima_keyrings_len = 0;
 				result = -ENOMEM;
 				break;
 			}
-- 
2.17.1

             reply	other threads:[~2020-01-16 23:46 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 23:46 Lakshmi Ramasubramanian [this message]
2020-01-16 23:46 ` [PATCH v1] IMA: pre-allocate buffer to hold keyrings string Lakshmi Ramasubramanian
2020-01-17  1:17 ` 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=20200116234623.2959-1-nramas@linux.microsoft.com \
    --to=nramas@linux.microsoft.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=dhowells@redhat.com \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sashal@kernel.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.