On Fri, Sep 11, 2020 at 1:56 AM Kees Cook [off-list ref] wrote:

On Thu, Sep 10, 2020 at 01:21:07PM -0700, Kees Cook wrote:

quoted

From: John Wood <redacted>

In order to mitigate a fork brute force attack it is necessary to kill
all the offending tasks. This tasks are all the ones that share the
statistical data with the current task (the task that has crashed).

Since the attack detection is done in the function fbfam_handle_attack()
that is called every time a core dump is triggered, only is needed to
kill the others tasks that share the same statistical data, not the
current one as this is in the path to be killed.

[...]

quoted

+     for_each_process(p) {
+             if (p == current || p->fbfam_stats != stats)
+                     continue;
+
+             do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID);
+             pr_warn("fbfam: Offending process with PID %d killed\n",
+                     p->pid);

[...]

quoted

+
+             killed += 1;
+             if (killed >= to_kill)
+                     break;
+     }
+
+     rcu_read_unlock();

Can't newly created processes escape this RCU read lock? I think this
need alternate locking, or something in the task_alloc hook that will
block any new process from being created within the stats group.

Good point; the proper way to deal with this would probably be to take
the tasklist_lock in read mode around this loop (with
read_lock(&tasklist_lock) / read_unlock(&tasklist_lock)), which pairs
with the write_lock_irq(&tasklist_lock) in copy_process(). Thanks to
the fatal_signal_pending() check while holding the lock in
copy_process(), that would be race-free - any fork() that has not yet
inserted the new task into the global task list would wait for us to
drop the tasklist_lock, then bail out at the fatal_signal_pending()
check.