Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack

From: John Wood <john.wood@gmx.com>
To: Kees Cook <keescook@chromium.org>
Cc: John Wood <john.wood@gmx.com>, Jann Horn <jannh@google.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Jonathan Corbet <corbet@lwn.net>,
	James Morris <jmorris@namei.org>, Shuah Khan <shuah@kernel.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andi Kleen <ak@linux.intel.com>,
	kernel test robot <oliver.sang@intel.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	kernel-hardening@lists.openwall.com
Subject: Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
Date: Sun, 21 Mar 2021 16:01:18 +0100	[thread overview]
Message-ID: <20210321150118.GA3403@ubuntu> (raw)
In-Reply-To: <202103171902.E6F55172@keescook>

On Wed, Mar 17, 2021 at 07:57:10PM -0700, Kees Cook wrote:
> On Sun, Mar 07, 2021 at 12:30:26PM +0100, John Wood wrote:
> > +static u64 brute_update_crash_period(struct brute_stats *stats, u64 now)
> > +{
> > +	u64 current_period;
> > +	u64 last_crash_timestamp;
> > +
> > +	spin_lock(&stats->lock);
> > +	current_period = now - stats->jiffies;
> > +	last_crash_timestamp = stats->jiffies;
> > +	stats->jiffies = now;
> > +
> > +	stats->period -= brute_mul_by_ema_weight(stats->period);
> > +	stats->period += brute_mul_by_ema_weight(current_period);
> > +
> > +	if (stats->faults < BRUTE_MAX_FAULTS)
> > +		stats->faults += 1;
> > +
> > +	spin_unlock(&stats->lock);
> > +	return last_crash_timestamp;
> > +}
>
> Now *here* locking makes sense, and it only needs to be per-stat, not
> global, since multiple processes may be operating on the same stat
> struct. To make this more no-reader-locking-friendly, I'd also update
> everything at the end, and use WRITE_ONCE():
>
> 	u64 current_period, period;
> 	u64 last_crash_timestamp;
> 	u64 faults;
>
> 	spin_lock(&stats->lock);
> 	current_period = now - stats->jiffies;
> 	last_crash_timestamp = stats->jiffies;
>
> 	WRITE_ONCE(stats->period,
> 		   stats->period - brute_mul_by_ema_weight(stats->period) +
> 		   brute_mul_by_ema_weight(current_period));
>
> 	if (stats->faults < BRUTE_MAX_FAULTS)
> 		WRITE_ONCE(stats->faults, stats->faults + 1);
>
> 	WRITE_ONCE(stats->jiffies, now);
>
> 	spin_unlock(&stats->lock);
> 	return last_crash_timestamp;
>
> That way readers can (IIUC) safely use READ_ONCE() on jiffies and faults
> without needing to hold the &stats->lock (unless they need perfectly matching
> jiffies, period, and faults).

Sorry, but I try to understand how to use locking properly without luck.

I have read (and tried to understand):
   tools/memory-model/Documentation/simple.txt
   tools/memory-model/Documentation/ordering.txt
   tools/memory-model/Documentation/recipes.txt
   Documentation/memory-barriers.txt

And I don't find the responses that I need. I'm not saying they aren't
there but I don't see them. So my questions:

If in the above function makes sense to use locking, and it is called from
the brute_task_fatal_signal hook, then, all the functions that are called
from this hook need locking (more than one process can access stats at the
same time).

So, as you point, how it is possible and safe to read jiffies and faults
(and I think period even though you not mention it) using READ_ONCE() but
without holding brute_stats::lock? I'm very confused.

IIUC (during the reading of the documentation) READ_ONCE and WRITE_ONCE only
guarantees that a variable loaded with WRITE_ONCE can be read safely with
READ_ONCE avoiding tearing, etc. So, I see these functions like a form of
guarantee atomicity in variables.

Another question. Is it also safe to use WRITE_ONCE without holding the lock?
Or this is only appliable to read operations?

Any light on this will help me to do the best job in the next patches. If
somebody can point me to the right direction it would be greatly appreciated.

Is there any documentation for newbies regarding this theme? I'm stuck.
I have also read the documentation about spinlocks, semaphores, mutex, etc..
but nothing clears me the concept expose.

Apologies if this question has been answered in the past. But the search in
the mailing list has not been lucky.

Thanks for your time and patience.
John Wood