On Wed, Mar 17, 2021 at 09:04:15PM -0700, Kees Cook wrote:
> On Sun, Mar 07, 2021 at 12:30:28PM +0100, John Wood wrote:
> > +/**
> > + * brute_kill_offending_tasks() - Kill the offending tasks.
> > + * @attack_type: Brute force attack type.
> > + * @stats: Statistical data shared by all the fork hierarchy processes.
> > + *
> > + * When a brute force attack is detected all the offending tasks involved in the
> > + * attack must be killed. In other words, it is necessary to kill all the tasks
> > + * that share the same statistical data. Moreover, if the attack happens through
> > + * the fork system call, the processes that have the same group_leader that the
> > + * current task must be avoided since they are in the path to be killed.
> > + *
> > + * When the SIGKILL signal is sent to the offending tasks, this function will be
> > + * called again from the task_fatal_signal hook due to a small crash period. So,
> > + * to avoid kill again the same tasks due to a recursive call of this function,
> > + * it is necessary to disable the attack detection for this fork hierarchy.
>
> Hah. Interesting. I wonder if there is a better way to handle this. Hmm.
If your comment is related to disable the detection:
I think it's no problematic to disable the attack detection for this fork
hierarchy since all theirs tasks will be removed. Also, I think that the disable
mark can help in the path to use the wait*() functions to notify userspace that
a task has been killed by the brute mitigation. Is a work in progress now.
If your comment is related to kill all the tasks:
In the previous version I have a useful discussion with Andi Kleen where a
proposal to block the fork system call during a time was made. He explains me
the cons of this method and proposes that if the mitigation works as now we can
use the wait*() functions to notify userspace that the tasks has been killed
by the brute mitigation. This way other problems related with the supervisors
and respawned processes could be handled.
Anyway, new points of view are also welcome.
> > + *
> > + * The statistical data shared by all the fork hierarchy processes cannot be
> > + * NULL.
> > + *
> > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock
> > + * since the task_free hook can be called from an IRQ context during the
> > + * execution of the task_fatal_signal hook.
> > + *
> > + * Context: Must be called with interrupts disabled and tasklist_lock and
> > + * brute_stats_ptr_lock held.
> > + */
> > +static void brute_kill_offending_tasks(enum brute_attack_type attack_type,
> > + struct brute_stats *stats)
> > +{
> > + struct task_struct *p;
> > + struct brute_stats **p_stats;
> > +
> > + spin_lock(&stats->lock);
> > +
> > + if (attack_type == BRUTE_ATTACK_TYPE_FORK &&
> > + refcount_read(&stats->refc) == 1) {
> > + spin_unlock(&stats->lock);
> > + return;
> > + }
>
> refcount_read() isn't a safe way to check that there is only 1
> reference. What's this trying to do?
If a fork brute force attack has been detected is due to a new fatal crash.
Under this scenario, if there is only one reference of these stats, it is
not necessary to kill any other tasks since the stats are not shared with
another process. Moreover, if this task has failed in a fatal way, is in
the path to be killed. So, no action is required.
How can I make this check in a safe way?
> > +
> > + brute_disable(stats);
> > + spin_unlock(&stats->lock);
> > +
> > + for_each_process(p) {
> > + if (attack_type == BRUTE_ATTACK_TYPE_FORK &&
> > + p->group_leader == current->group_leader)
> > + continue;
> > +
> > + p_stats = brute_stats_ptr(p);
> > + if (*p_stats != stats)
> > + continue;
> > +
> > + do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID);
> > + pr_warn_ratelimited("Offending process %d [%s] killed\n",
> > + p->pid, p->comm);
> > + }
> > +}
Thanks,
John Wood
