Skip to contentSkip to conversation

IMA: Use delayed work to free queued keys

PATCH In review Lakshmi Ramasubramanian 2 participants 2 messages Jan 21, 2020 – Jan 22, 2020

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 IMA: Use delayed work to free queued keys

A timer is used to free queued keys if a custom IMA policy is not loaded within 5 minutes after IMA subsystem is initialized. Timer handler is called in interrupt context. Due to this a spinlock has to be used to synchronize access to critical section. A mutex cannot be used since a mutex can sleep.

This patch uses a delayed work to free queued keys. Since a delayed work handler is called in process context a mutex can be used.

Fixes 8f5d2d06f217 ("IMA: Defined timer to free queued keys")
---
 security/integrity/ima/ima_asymmetric_keys.c | 33 ++++++++++----------
 1 file changed, 16 insertions(+), 17 deletions(-)
security/integrity/ima/ima_asymmetric_keys.c +16 -17
index 381f51708e7b..fa1bdd54a9ff 100644
@@ -11,7 +11,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/timer.h>
+#include <linux/workqueue.h>
 #include <keys/asymmetric-type.h>
 #include "ima.h"
 
@@ -24,37 +24,37 @@ static bool ima_process_keys;
 /*
  * To synchronize access to the list of keys that need to be measured
  */
-static DEFINE_SPINLOCK(ima_keys_lock);
+static DEFINE_MUTEX(ima_keys_lock);
 static LIST_HEAD(ima_keys);
 
 /*
  * If custom IMA policy is not loaded then keys queued up
- * for measurement should be freed. This timer is used
+ * for measurement should be freed. This worker is used
  * for handling this scenario.
  */
 static long ima_key_queue_timeout = 300000; /* 5 Minutes */
-static struct timer_list ima_key_queue_timer;
+static void ima_keys_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler);
 static bool timer_expired;
 
 /*
- * This timer callback function frees keys that may still be
+ * This worker function frees keys that may still be
  * queued up in case custom IMA policy was not loaded.
  */
-static void ima_timer_handler(struct timer_list *timer)
+static void ima_keys_handler(struct work_struct *work)
 {
 	timer_expired = true;
 	ima_process_queued_keys();
 }
 
 /*
- * This function sets up a timer to free queued keys in case
+ * This function sets up a worker to free queued keys in case
  * custom IMA policy was never loaded.
  */
 void ima_init_key_queue(void)
 {
-	timer_setup(&ima_key_queue_timer, ima_timer_handler, 0);
-	mod_timer(&ima_key_queue_timer,
-		  jiffies + msecs_to_jiffies(ima_key_queue_timeout));
+	schedule_delayed_work(&ima_keys_delayed_work,
+			      msecs_to_jiffies(ima_key_queue_timeout));
 }
 
 static void ima_free_key_entry(struct ima_key_entry *entry)
@@ -103,18 +103,17 @@ static bool ima_queue_key(struct key *keyring, const void *payload,
 {
 	bool queued = false;
 	struct ima_key_entry *entry;
-	unsigned long flags;
 
 	entry = ima_alloc_key_entry(keyring, payload, payload_len);
 	if (!entry)
 		return false;
 
-	spin_lock_irqsave(&ima_keys_lock, flags);
+	mutex_lock(&ima_keys_lock);
 	if (!ima_process_keys) {
 		list_add_tail(&entry->list, &ima_keys);
 		queued = true;
 	}
-	spin_unlock_irqrestore(&ima_keys_lock, flags);
+	mutex_unlock(&ima_keys_lock);
 
 	if (!queued)
 		ima_free_key_entry(entry);
@@ -132,7 +131,6 @@ void ima_process_queued_keys(void)
 {
 	struct ima_key_entry *entry, *tmp;
 	bool process = false;
-	unsigned long flags;
 
 	if (ima_process_keys)
 		return;
@@ -143,17 +141,18 @@ void ima_process_queued_keys(void)
 	 * First one setting the ima_process_keys flag to true will
 	 * process the queued keys.
 	 */
-	spin_lock_irqsave(&ima_keys_lock, flags);
+	mutex_lock(&ima_keys_lock);
 	if (!ima_process_keys) {
 		ima_process_keys = true;
 		process = true;
 	}
-	spin_unlock_irqrestore(&ima_keys_lock, flags);
+	mutex_unlock(&ima_keys_lock);
 
 	if (!process)
 		return;
 
-	del_timer(&ima_key_queue_timer);
+	if (!timer_expired)
+		cancel_delayed_work_sync(&ima_keys_delayed_work);
 
 	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
 		if (!timer_expired)
Signature · 1 line
2.17.1
maintainer
#1 lore raw

Hi Lackshmi,

In the subject line and the patch description please refer to the construct as "delayed workqueue" or even abbreviated as "delayed wq", even though the struct itself is named "delayed_work.

Quoted 1 line · Lakshmi Ramasubramanian

A timer is used to free queued keys if a custom IMA policy is not

^to free the queued keys,

Quoted 1 line

loaded within 5 minutes after IMA subsystem is initialized. Timer

^after the IMA subsystem
^The timer handler

Quoted 9 lines

handler is called in interrupt context. Due to this a spinlock has to be used to synchronize access to critical section. A mutex cannot be used since a mutex can sleep.

This patch uses a delayed work to free queued keys. Since a delayed work handler is called in process context a mutex can be used.

Fixes 8f5d2d06f217 ("IMA: Defined timer to free queued keys")

Thank you.  This is definitely better.

Mimi

Quoted 115 lines
---
 security/integrity/ima/ima_asymmetric_keys.c | 33 ++++++++++----------
 1 file changed, 16 insertions(+), 17 deletions(-)
security/integrity/ima/ima_asymmetric_keys.c +16 -17
index 381f51708e7b..fa1bdd54a9ff 100644
@@ -11,7 +11,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/timer.h>
+#include <linux/workqueue.h>
 #include <keys/asymmetric-type.h>
 #include "ima.h"
 
@@ -24,37 +24,37 @@ static bool ima_process_keys;
 /*
  * To synchronize access to the list of keys that need to be measured
  */
-static DEFINE_SPINLOCK(ima_keys_lock);
+static DEFINE_MUTEX(ima_keys_lock);
 static LIST_HEAD(ima_keys);
 
 /*
  * If custom IMA policy is not loaded then keys queued up
- * for measurement should be freed. This timer is used
+ * for measurement should be freed. This worker is used
  * for handling this scenario.
  */
 static long ima_key_queue_timeout = 300000; /* 5 Minutes */
-static struct timer_list ima_key_queue_timer;
+static void ima_keys_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler);
 static bool timer_expired;
 
 /*
- * This timer callback function frees keys that may still be
+ * This worker function frees keys that may still be
  * queued up in case custom IMA policy was not loaded.
  */
-static void ima_timer_handler(struct timer_list *timer)
+static void ima_keys_handler(struct work_struct *work)
 {
 	timer_expired = true;
 	ima_process_queued_keys();
 }
 
 /*
- * This function sets up a timer to free queued keys in case
+ * This function sets up a worker to free queued keys in case
  * custom IMA policy was never loaded.
  */
 void ima_init_key_queue(void)
 {
-	timer_setup(&ima_key_queue_timer, ima_timer_handler, 0);
-	mod_timer(&ima_key_queue_timer,
-		  jiffies + msecs_to_jiffies(ima_key_queue_timeout));
+	schedule_delayed_work(&ima_keys_delayed_work,
+			      msecs_to_jiffies(ima_key_queue_timeout));
 }
 
 static void ima_free_key_entry(struct ima_key_entry *entry)
@@ -103,18 +103,17 @@ static bool ima_queue_key(struct key *keyring, const void *payload,
 {
 	bool queued = false;
 	struct ima_key_entry *entry;
-	unsigned long flags;
 
 	entry = ima_alloc_key_entry(keyring, payload, payload_len);
 	if (!entry)
 		return false;
 
-	spin_lock_irqsave(&ima_keys_lock, flags);
+	mutex_lock(&ima_keys_lock);
 	if (!ima_process_keys) {
 		list_add_tail(&entry->list, &ima_keys);
 		queued = true;
 	}
-	spin_unlock_irqrestore(&ima_keys_lock, flags);
+	mutex_unlock(&ima_keys_lock);
 
 	if (!queued)
 		ima_free_key_entry(entry);
@@ -132,7 +131,6 @@ void ima_process_queued_keys(void)
 {
 	struct ima_key_entry *entry, *tmp;
 	bool process = false;
-	unsigned long flags;
 
 	if (ima_process_keys)
 		return;
@@ -143,17 +141,18 @@ void ima_process_queued_keys(void)
 	 * First one setting the ima_process_keys flag to true will
 	 * process the queued keys.
 	 */
-	spin_lock_irqsave(&ima_keys_lock, flags);
+	mutex_lock(&ima_keys_lock);
 	if (!ima_process_keys) {
 		ima_process_keys = true;
 		process = true;
 	}
-	spin_unlock_irqrestore(&ima_keys_lock, flags);
+	mutex_unlock(&ima_keys_lock);
 
 	if (!process)
 		return;
 
-	del_timer(&ima_key_queue_timer);
+	if (!timer_expired)
+		cancel_delayed_work_sync(&ima_keys_delayed_work);
 
 	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
 		if (!timer_expired)