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@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