Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Sep 2020 18:02:16 +0200
From:   John Wood <john.wood@gmx.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     kernel-hardening@lists.openwall.com, Jann Horn <jannh@google.com>,
        John Wood <john.wood@gmx.com>,
        Matthew Wilcox <willy@infradead.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-security-module@vger.kernel.org
Subject: Re: [RFC PATCH 6/6] security/fbfam: Mitigate a fork brute force
 attack
Message-ID: <20200918152116.GB3229@ubuntu>
References: <20200910202107.3799376-1-keescook@chromium.org>
 <20200910202107.3799376-7-keescook@chromium.org>
 <202009101649.2A0BF95@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202009101649.2A0BF95@keescook>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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_a=
ttack(),
> > + * and so, every time a core dump is triggered, only is needed to kil=
l 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 funct=
ion will be
> > + * called again during the core dump due to the shared statistical da=
ta 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 at=
tack
> > + * 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 =3D current->fbfam_stats;
> > +	struct task_struct *p;
> > +	unsigned int to_kill, killed =3D 0;
> > +
> > +	if (!stats)
> > +		return -EFAULT;
> > +
> > +	to_kill =3D refcount_read(&stats->refc) - 1;
> > +	if (!to_kill)
> > +		return 0;
> > +
> > +	/* Disable the attack detection */
> > +	stats->jiffies =3D 0;
> > +	rcu_read_lock();
> > +
> > +	for_each_process(p) {
> > +		if (p =3D=3D current || p->fbfam_stats !=3D 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 +=3D 1;
> > +		if (killed >=3D 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