[RFC PATCH 2/2] security/brute.c: Protect the stats pointer

[John Wood]

I think the stats pointer present in the task_struct's security blob
needs to be protected against concurrency for the following reasons.

1.- The same process forking at the same time in two different CPUs.
2.- The same process execve() at the same time in two different CPUs.
3.- A process forking or execve() while we traverse the fork hierarchy
    in the brute_get_exec_stats() function.
4.- More cases that now not come to my mind.

So, the question is: Is the locking used correct?

I use a read/write spinlock to allow multiple readers when the stats
pointer not change. But when this pointer needs to be updated, the
access must be exclusive by only one writer.

Moreover, the kmalloc() uses now the GFP_ATOMIC option to avoid sleep
in an atomic context. Is this a problem? It would be better allocate the
memory for the stats structure outside of an atomic context using
GFP_KERNEL and update the stats pointer inside the atomic context? This
will make necessary always allocate memory (and free it if its not
necessary) in the task_alloc hook.

Also I would like to know if is it necessary a more fine grained
locking? I think that it is not possible because we need to avoid that,
for example, if the same process is execve() at the same time in two
different CPUs, that two stat structures are allocated and then only
one assigned to the shared stats pointer. I think that in this situation
will be a memory leak. I also believe that the other cases cannot use a
more fine grained locking since we need that this sections are atomic to
avoid inconsistence states like the above commented.

Another question. If I use this lock to protect the stats pointer, can
be the stats->lock avoided (to acquire and release) in the
brute_share_stats function and the task_execve function? I think that in
this scenario the brute_stats_ptr_lock lock also protects the access to
the internal data of stats structure.

But I think that the stats->lock cannot be removed completely since in the
task_fatal_signal function the brute_stats_ptr_lock is acquired in read
state but we need to write to the stats structure to compute the crash
period.

Any constructive comments are welcome. I would like to know your
opinions about my locking system but remember that I'm a newbie :)

Signed-off-by: John Wood <john.wood at gmx.com>
---
 security/brute/brute.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/security/brute/brute.c b/security/brute/brute.c
index 60944a0f8de8..926bffb61a3b 100644
--- a/security/brute/brute.c
+++ b/security/brute/brute.c
@@ -18,6 +18,8 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>

+static DEFINE_RWLOCK(brute_stats_ptr_lock);
+
 /**
  * struct brute_stats - Fork brute force attack statistics.
  * @lock: Lock to protect the brute_stats structure.
@@ -74,7 +76,7 @@ static struct brute_stats *brute_new_stats(void)
 {
 	struct brute_stats *stats;

-	stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
+	stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC);
 	if (!stats)
 		return NULL;

@@ -135,17 +137,22 @@ static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags)

 	stats = brute_stats_ptr(task);
 	p_stats = brute_stats_ptr(current);
+	write_lock(&brute_stats_ptr_lock);

 	if (likely(*p_stats)) {
 		brute_share_stats(p_stats, stats);
+		write_unlock(&brute_stats_ptr_lock);
 		return 0;
 	}

 	*stats = brute_new_stats();
-	if (!*stats)
+	if (!*stats) {
+		write_unlock(&brute_stats_ptr_lock);
 		return -ENOMEM;
+	}

 	brute_share_stats(stats, p_stats);
+	write_unlock(&brute_stats_ptr_lock);
 	return 0;
 }

@@ -177,8 +184,12 @@ static void brute_task_execve(struct linux_binprm *bprm)
 	unsigned long flags;

 	stats = brute_stats_ptr(current);
-	if (WARN(!*stats, "No statistical data\n"))
+	write_lock(&brute_stats_ptr_lock);
+
+	if (WARN(!*stats, "No statistical data\n")) {
+		write_unlock(&brute_stats_ptr_lock);
 		return;
+	}

 	spin_lock_irqsave(&(*stats)->lock, flags);

@@ -188,6 +199,7 @@ static void brute_task_execve(struct linux_binprm *bprm)
 		(*stats)->jiffies = get_jiffies_64();
 		(*stats)->period = 0;
 		spin_unlock_irqrestore(&(*stats)->lock, flags);
+		write_unlock(&brute_stats_ptr_lock);
 		return;
 	}

@@ -195,6 +207,7 @@ static void brute_task_execve(struct linux_binprm *bprm)
 	spin_unlock_irqrestore(&(*stats)->lock, flags);
 	*stats = brute_new_stats();
 	WARN(!*stats, "Cannot allocate statistical data\n");
+	write_unlock(&brute_stats_ptr_lock);
 }

 /**
@@ -210,8 +223,12 @@ static void brute_task_free(struct task_struct *task)
 	bool refc_is_zero;

 	stats = brute_stats_ptr(task);
-	if (WARN(!*stats, "No statistical data\n"))
+	read_lock(&brute_stats_ptr_lock);
+
+	if (WARN(!*stats, "No statistical data\n")) {
+		read_unlock(&brute_stats_ptr_lock);
 		return;
+	}

 	spin_lock(&(*stats)->lock);
 	refc_is_zero = refcount_dec_and_test(&(*stats)->refc);
@@ -219,6 +236,8 @@ static void brute_task_free(struct task_struct *task)

 	if (refc_is_zero)
 		kfree(*stats);
+
+	read_unlock(&brute_stats_ptr_lock);
 }

 static const u64 BRUTE_EMA_WEIGHT_NUMERATOR = 7;
@@ -313,8 +332,10 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo)
 	u64 exec_period;

 	stats = brute_stats_ptr(current);
+	read_lock(&brute_stats_ptr_lock);
+
 	if (WARN(!*stats, "No statistical data\n"))
-		return;
+		goto unlock;

 	brute_get_crash_periods(*stats, &fork_period, &exec_period);

@@ -322,10 +343,12 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo)
 		pr_warn("Brute force attack detected through fork\n");

 	if (fork_period == exec_period)
-		return;
+		goto unlock;

 	if (exec_period && exec_period < BRUTE_EMA_CRASH_PERIOD_THRESHOLD)
 		pr_warn("Brute force attack detected through execve\n");
+unlock:
+	read_unlock(&brute_stats_ptr_lock);
 }

 /**
--
2.25.1