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

[John Wood]

On Tue, 08 Dec 2020 09:42:59 -0500, Valdis Klētnieks wrote:
> On Tue, 08 Dec 2020 11:35:57 +0100, John Wood said:
> > 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.
>
> OK, I'll bite.  How would these two cases even happen?
>
> (Note that you could conceivably issue the fork()/exeve() on one CPU, run
> kernel code for a bit and then get rescheduled onto a different CPU to complete
> the syscall, but that's a different cache coherency can-o-worms :)

Thanks for the reply. Understood. The first and second cases can never happen.

> (Your case 3 of a fork/exec while you traverse is an actual issue.  Note that
> you missed one case - where the process evaporates for some reason while you do
> the traverse and you're left with a stale pointer...)

Ok, so I still need protection for the stats pointer.

Since the 1 and 2 cases can never happen, I believe that there is no need
to make all the fork, execve and free management atomic. In other words,
now I think I can protect the reading and the writing operations
separately. Note that the fork management is atomic because basically all
the operations are writing and I believe that this way is better than
acquire the lock in read state, release it, acquire in write state,
release it, acquire again in read state, ...

Also, to deal with the case where a process evaporates while I do the
traverse, I set the pointer to NULL after free it. This way I can notice
this state.

This is the protection system now. What do you think?
I hope you are not too hard on my code :)

---
 security/brute/brute.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/security/brute/brute.c b/security/brute/brute.c
index 60944a0f8de8..0c6bebd9bf18 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"))
+	read_lock(&brute_stats_ptr_lock);
+
+	if (WARN(!*stats, "No statistical data\n")) {
+		read_unlock(&brute_stats_ptr_lock);
 		return;
+	}

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

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

 	/* execve call after a fork call */
 	spin_unlock_irqrestore(&(*stats)->lock, flags);
+	read_unlock(&brute_stats_ptr_lock);
+
+	write_lock(&brute_stats_ptr_lock);
 	*stats = brute_new_stats();
 	WARN(!*stats, "Cannot allocate statistical data\n");
+	write_unlock(&brute_stats_ptr_lock);
 }

 /**
@@ -210,15 +226,24 @@ 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);
 	spin_unlock(&(*stats)->lock);
+	read_unlock(&brute_stats_ptr_lock);

-	if (refc_is_zero)
+	if (refc_is_zero) {
+		write_lock(&brute_stats_ptr_lock);
 		kfree(*stats);
+		*stats = NULL;
+		write_unlock(&brute_stats_ptr_lock);
+	}
 }

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

 	stats = brute_stats_ptr(current);
-	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;
+	}

 	brute_get_crash_periods(*stats, &fork_period, &exec_period);
+	read_unlock(&brute_stats_ptr_lock);

 	if (fork_period && fork_period < BRUTE_EMA_CRASH_PERIOD_THRESHOLD)
 		pr_warn("Brute force attack detected through fork\n");
--
2.25.1