[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 18 Sep 2020 18:02:16 +0200
From: John Wood <john.wood@....com>
To: Kees Cook <keescook@...omium.org>
Cc: kernel-hardening@...ts.openwall.com, Jann Horn <jannh@...gle.com>,
John Wood <john.wood@....com>, Matthew Wilcox <willy@...radead.org>,
Jonathan Corbet <corbet@....net>,
Alexander Viro <viro@...iv.linux.org.uk>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>,
Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
Luis Chamberlain <mcgrof@...nel.org>,
Iurii Zaikin <yzaikin@...gle.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH 6/6] security/fbfam: Mitigate a fork brute force
attack
On Thu, Sep 10, 2020 at 04:56:19PM -0700, Kees Cook wrote:
> On Thu, Sep 10, 2020 at 01:21:07PM -0700, Kees Cook wrote:
> > /**
> > + * fbfam_kill_tasks() - Kill the offending tasks
> > + *
> > + * When a fork brute force attack is detected it is necessary to kill all the
> > + * offending tasks. Since this function is called from fbfam_handle_attack(),
> > + * and so, 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.
> > + *
> > + * When the SIGKILL signal is sent to the offending tasks, this function will be
> > + * called again during the core dump due to the shared statistical data shows a
> > + * quickly crashing rate. So, to avoid kill again the same tasks due to a
> > + * recursive call of this function, it is necessary to disable the attack
> > + * detection setting the jiffies to zero.
> > + *
> > + * To improve the for_each_process loop it is possible to end it when all the
> > + * tasks that shared the same statistics are found.
> > + *
> > + * Return: -EFAULT if the current task doesn't have statistical data. Zero
> > + * otherwise.
> > + */
> > +static int fbfam_kill_tasks(void)
> > +{
> > + struct fbfam_stats *stats = current->fbfam_stats;
> > + struct task_struct *p;
> > + unsigned int to_kill, killed = 0;
> > +
> > + if (!stats)
> > + return -EFAULT;
> > +
> > + to_kill = refcount_read(&stats->refc) - 1;
> > + if (!to_kill)
> > + return 0;
> > +
> > + /* Disable the attack detection */
> > + stats->jiffies = 0;
> > + rcu_read_lock();
> > +
> > + 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);
>
> I'd make this ratelimited (along with Jann's suggestions).
Sorry, but I don't understand what you mean with "make this ratelimited".
A clarification would be greatly appreciated.
> Also, instead of the explicit "fbfam:" prefix, use the regular
> prefixing method:
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Understood.
> > +
> > + 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.
I will work on this for the next version. Thanks.
> > + return 0;
> > +}
>
> --
> Kees Cook
Thanks
John Wood
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
