Message-ID: <202103171902.E6F55172@keescook>
Date:   Wed, 17 Mar 2021 19:57:10 -0700
From:   Kees Cook <keescook@...omium.org>
To:     John Wood <john.wood@....com>
Cc:     Jann Horn <jannh@...gle.com>, Randy Dunlap <rdunlap@...radead.org>,
        Jonathan Corbet <corbet@....net>,
        James Morris <jmorris@...ei.org>,
        Shuah Khan <shuah@...nel.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andi Kleen <ak@...ux.intel.com>,
        kernel test robot <oliver.sang@...el.com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack

On Sun, Mar 07, 2021 at 12:30:26PM +0100, John Wood wrote:
> To detect a brute force attack it is necessary that the statistics
> shared by all the fork hierarchy processes be updated in every fatal
> crash and the most important data to update is the application crash
> period. To do so, use the new "task_fatal_signal" LSM hook added in a
> previous step.
> 
> The application crash period must be a value that is not prone to change
> due to spurious data and follows the real crash period. So, to compute
> it, the exponential moving average (EMA) is used.
> 
> There are two types of brute force attacks that need to be detected. The
> first one is an attack that happens through the fork system call and the
> second one is an attack that happens through the execve system call. The
> first type uses the statistics shared by all the fork hierarchy
> processes, but the second type cannot use this statistical data due to
> these statistics disappear when the involved tasks finished. In this
> last scenario the attack info should be tracked by the statistics of a
> higher fork hierarchy (the hierarchy that contains the process that
> forks before the execve system call).
> 
> Moreover, these two attack types have two variants. A slow brute force
> attack that is detected if the maximum number of faults per fork
> hierarchy is reached and a fast brute force attack that is detected if
> the application crash period falls below a certain threshold.
> 
> Also, this patch adds locking to protect the statistics pointer hold by
> every process.
> 
> Signed-off-by: John Wood <john.wood@....com>
> ---
>  security/brute/brute.c | 498 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 479 insertions(+), 19 deletions(-)
> 
> diff --git a/security/brute/brute.c b/security/brute/brute.c
> index 99d099e45112..870db55332d4 100644
> --- a/security/brute/brute.c
> +++ b/security/brute/brute.c
> @@ -11,9 +11,14 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/lsm_hooks.h>
> +#include <linux/math64.h>
>  #include <linux/printk.h>
>  #include <linux/refcount.h>
> +#include <linux/rwlock.h>
> +#include <linux/rwlock_types.h>
>  #include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/sched/task.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
> @@ -37,6 +42,11 @@ struct brute_stats {
>  	u64 period;
>  };
> 
> +/*
> + * brute_stats_ptr_lock - Lock to protect the brute_stats structure pointer.
> + */
> +static DEFINE_RWLOCK(brute_stats_ptr_lock);

Yeow, you've switched from an (unneeded in prior patch) per-stats lock
to a global lock? I think this isn't needed...

> +
>  /*
>   * brute_blob_sizes - LSM blob sizes.
>   *
> @@ -74,7 +84,7 @@ static struct brute_stats *brute_new_stats(void)
>  {
>  	struct brute_stats *stats;
> 
> -	stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
> +	stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC);

Why change this here? I'd just start with this in the patch that
introduces it.

>  	if (!stats)
>  		return NULL;
> 
> @@ -99,16 +109,17 @@ static struct brute_stats *brute_new_stats(void)
>   * 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_alloc hook.
> + *
> + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock
> + *          held.
>   */
>  static void brute_share_stats(struct brute_stats *src,
>  			      struct brute_stats **dst)
>  {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&src->lock, flags);
> +	spin_lock(&src->lock);
>  	refcount_inc(&src->refc);
>  	*dst = src;
> -	spin_unlock_irqrestore(&src->lock, flags);
> +	spin_unlock(&src->lock);
>  }

I still don't think any locking is needed here; the whole function can
go away, IMO.

> 
>  /**
> @@ -126,26 +137,36 @@ static void brute_share_stats(struct brute_stats *src,
>   * this task and the new one being allocated. Otherwise, share the statistics
>   * that the current task already has.
>   *
> + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> + * and brute_stats::lock since the task_free hook can be called from an IRQ
> + * context during the execution of the task_alloc hook.
> + *
>   * Return: -ENOMEM if the allocation of the new statistics structure fails. Zero
>   *         otherwise.
>   */
>  static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags)
>  {
>  	struct brute_stats **stats, **p_stats;
> +	unsigned long flags;
> 
>  	stats = brute_stats_ptr(task);
>  	p_stats = brute_stats_ptr(current);
> +	write_lock_irqsave(&brute_stats_ptr_lock, flags);
> 
>  	if (likely(*p_stats)) {
>  		brute_share_stats(*p_stats, stats);
> +		write_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  		return 0;
>  	}
> 
>  	*stats = brute_new_stats();
> -	if (!*stats)
> +	if (!*stats) {
> +		write_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  		return -ENOMEM;
> +	}
> 
>  	brute_share_stats(*stats, p_stats);
> +	write_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  	return 0;
>  }

I'd much prefer that whatever locking is needed be introduced in the
initial patch: this transformation just double the work to review. :)

> 
> @@ -167,9 +188,9 @@ static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags)
>   * only one task (the task that calls the execve function) points to the data.
>   * In this case, the previous allocation is used but the statistics are reset.
>   *
> - * 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 bprm_committing_creds hook.
> + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> + * and brute_stats::lock since the task_free hook can be called from an IRQ
> + * context during the execution of the bprm_committing_creds hook.
>   */
>  static void brute_task_execve(struct linux_binprm *bprm)
>  {
> @@ -177,24 +198,33 @@ static void brute_task_execve(struct linux_binprm *bprm)
>  	unsigned long flags;
> 
>  	stats = brute_stats_ptr(current);
> -	if (WARN(!*stats, "No statistical data\n"))
> +	read_lock_irqsave(&brute_stats_ptr_lock, flags);
> +
> +	if (WARN(!*stats, "No statistical data\n")) {
> +		read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  		return;
> +	}
> 
> -	spin_lock_irqsave(&(*stats)->lock, flags);
> +	spin_lock(&(*stats)->lock);
> 
>  	if (!refcount_dec_not_one(&(*stats)->refc)) {
>  		/* execve call after an execve call */
>  		(*stats)->faults = 0;
>  		(*stats)->jiffies = get_jiffies_64();
>  		(*stats)->period = 0;
> -		spin_unlock_irqrestore(&(*stats)->lock, flags);
> +		spin_unlock(&(*stats)->lock);
> +		read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  		return;
>  	}
> 
>  	/* execve call after a fork call */
> -	spin_unlock_irqrestore(&(*stats)->lock, flags);
> +	spin_unlock(&(*stats)->lock);
> +	read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> +
> +	write_lock_irqsave(&brute_stats_ptr_lock, flags);
>  	*stats = brute_new_stats();
>  	WARN(!*stats, "Cannot allocate statistical data\n");
> +	write_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  }

Again, I don't see a need for locking -- this is just managing the
lifetime which is entirely handled by the implicit locking of "current"
and the refcount_t.

> 
>  /**
> @@ -204,9 +234,9 @@ static void brute_task_execve(struct linux_binprm *bprm)
>   * The statistical data that is shared between all the fork hierarchy processes
>   * needs to be freed when this hierarchy disappears.
>   *
> - * 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_free hook.
> + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> + * and brute_stats::lock since the task_free hook can be called from an IRQ
> + * context during the execution of the task_free hook.
>   */
>  static void brute_task_free(struct task_struct *task)
>  {
> @@ -215,17 +245,446 @@ static void brute_task_free(struct task_struct *task)
>  	bool refc_is_zero;
> 
>  	stats = brute_stats_ptr(task);
> -	if (WARN(!*stats, "No statistical data\n"))
> +	read_lock_irqsave(&brute_stats_ptr_lock, flags);
> +
> +	if (WARN(!*stats, "No statistical data\n")) {
> +		read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
>  		return;
> +	}
> 
> -	spin_lock_irqsave(&(*stats)->lock, flags);
> +	spin_lock(&(*stats)->lock);
>  	refc_is_zero = refcount_dec_and_test(&(*stats)->refc);
> -	spin_unlock_irqrestore(&(*stats)->lock, flags);
> +	spin_unlock(&(*stats)->lock);
> +	read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> 
>  	if (refc_is_zero) {
> +		write_lock_irqsave(&brute_stats_ptr_lock, flags);
>  		kfree(*stats);
>  		*stats = NULL;
> +		write_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> +	}
> +}

Same; I would expect this to be simply:

	stats = brute_stats_ptr(task);
	if (WARN_ON_ONCE(!*stats))
		return;
	if (refcount_dec_and_test(&(*stats)->refc)) {
		kfree(*stats);
		*stats = NULL;
	}

> +
> +/*
> + * BRUTE_EMA_WEIGHT_NUMERATOR - Weight's numerator of EMA.
> + */
> +static const u64 BRUTE_EMA_WEIGHT_NUMERATOR = 7;
> +
> +/*
> + * BRUTE_EMA_WEIGHT_DENOMINATOR - Weight's denominator of EMA.
> + */
> +static const u64 BRUTE_EMA_WEIGHT_DENOMINATOR = 10;

Should these be externally configurable (via sysfs)?

> +
> +/**
> + * brute_mul_by_ema_weight() - Multiply by EMA weight.
> + * @value: Value to multiply by EMA weight.
> + *
> + * Return: The result of the multiplication operation.
> + */
> +static inline u64 brute_mul_by_ema_weight(u64 value)
> +{
> +	return mul_u64_u64_div_u64(value, BRUTE_EMA_WEIGHT_NUMERATOR,
> +				   BRUTE_EMA_WEIGHT_DENOMINATOR);
> +}
> +
> +/*
> + * BRUTE_MAX_FAULTS - Maximum number of faults.
> + *
> + * If a brute force attack is running slowly for a long time, the application
> + * crash period's EMA is not suitable for the detection. This type of attack
> + * must be detected using a maximum number of faults.
> + */
> +static const unsigned char BRUTE_MAX_FAULTS = 200;

Same.

> +
> +/**
> + * brute_update_crash_period() - Update the application crash period.
> + * @stats: Statistics that hold the application crash period to update.
> + * @now: The current timestamp in jiffies.
> + *
> + * The application crash period must be a value that is not prone to change due
> + * to spurious data and follows the real crash period. So, to compute it, the
> + * exponential moving average (EMA) is used.
> + *
> + * This kind of average defines a weight (between 0 and 1) for the new value to
> + * add and applies the remainder of the weight to the current average value.
> + * This way, some spurious data will not excessively modify the average and only
> + * if the new values are persistent, the moving average will tend towards them.
> + *
> + * Mathematically the application crash period's EMA can be expressed as
> + * follows:
> + *
> + * period_ema = period * weight + period_ema * (1 - weight)
> + *
> + * If the operations are applied:
> + *
> + * period_ema = period * weight + period_ema - period_ema * weight
> + *
> + * If the operands are ordered:
> + *
> + * period_ema = period_ema - period_ema * weight + period * weight
> + *
> + * Finally, this formula can be written as follows:
> + *
> + * period_ema -= period_ema * weight;
> + * period_ema += period * weight;
> + *
> + * The statistics that hold the application crash period to update 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 brute_stats_ptr_lock
> + *          held.
> + * Return: The last crash timestamp before updating it.
> + */
> +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).

> +
> +/*
> + * BRUTE_MIN_FAULTS - Minimum number of faults.
> + *
> + * The application crash period's EMA cannot be used until a minimum number of
> + * data has been applied to it. This constraint allows getting a trend when this
> + * moving average is used. Moreover, it avoids the scenario where an application
> + * fails quickly from execve system call due to reasons unrelated to a real
> + * attack.
> + */
> +static const unsigned char BRUTE_MIN_FAULTS = 5;
> +
> +/*
> + * BRUTE_CRASH_PERIOD_THRESHOLD - Application crash period threshold.
> + *
> + * The units are expressed in milliseconds.
> + *
> + * A fast brute force attack is detected when the application crash period falls
> + * below this threshold.
> + */
> +static const u64 BRUTE_CRASH_PERIOD_THRESHOLD = 30000;

These could all be sysctls (see yama's use of sysctl).

> +
> +/**
> + * brute_attack_running() - Test if a brute force attack is happening.
> + * @stats: Statistical data shared by all the fork hierarchy processes.
> + *
> + * The decision if a brute force attack is running is based on the statistical
> + * data shared by all the fork hierarchy processes. This statistics cannot be
> + * NULL.
> + *
> + * There are two types of brute force attacks that can be detected using the
> + * statistical data. The first one is a slow brute force attack that is detected
> + * if the maximum number of faults per fork hierarchy is reached. The second
> + * type is a fast brute force attack that is detected if the application crash
> + * period falls below a certain threshold.
> + *
> + * Moreover, it is important to note that no attacks will be detected until a
> + * minimum number of faults have occurred. This allows to have a trend in the
> + * crash period when the EMA is used and also avoids the scenario where an
> + * application fails quickly from execve system call due to reasons unrelated to
> + * a real attack.
> + *
> + * 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 brute_stats_ptr_lock
> + *          held.
> + * Return: True if a brute force attack is happening. False otherwise.
> + */
> +static bool brute_attack_running(struct brute_stats *stats)
> +{
> +	u64 crash_period;
> +
> +	spin_lock(&stats->lock);
> +	if (stats->faults < BRUTE_MIN_FAULTS) {
> +		spin_unlock(&stats->lock);
> +		return false;
> +	}

If I'm reading this correctly, you're performing two tests, so there
isn't a strict relationship between faults and period for this test,
and I think it could be done without locking with READ_ONCE():

	u64 faults;
	u64 crash_period;

	faults = READ_ONCE(stats->faults);
	if (faults < BRUTE_MIN_FAULTS)
		return false;
	if (faults >= BRUTE_MAX_FAULTS)
		return true;

	crash_period = jiffies64_to_msecs(READ_ONCE(stats->period));
	return crash_period < BRUTE_CRASH_PERIOD_THRESHOLD;


> +
> +	if (stats->faults >= BRUTE_MAX_FAULTS) {
> +		spin_unlock(&stats->lock);
> +		return true;
> +	}
> +
> +	crash_period = jiffies64_to_msecs(stats->period);
> +	spin_unlock(&stats->lock);
> +
> +	return crash_period < BRUTE_CRASH_PERIOD_THRESHOLD;
> +}
> +
> +/**
> + * print_fork_attack_running() - Warn about a fork brute force attack.
> + */
> +static inline void print_fork_attack_running(void)
> +{
> +	pr_warn("Fork brute force attack detected [%s]\n", current->comm);
> +}

I think pid should be part of this...

> +
> +/**
> + * brute_manage_fork_attack() - Manage a fork brute force attack.
> + * @stats: Statistical data shared by all the fork hierarchy processes.
> + * @now: The current timestamp in jiffies.
> + *
> + * For a correct management of a fork brute force attack it is only necessary to
> + * update the statistics and test if an attack is happening based on these data.
> + *
> + * 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 brute_stats_ptr_lock
> + *          held.
> + * Return: The last crash timestamp before updating it.
> + */
> +static u64 brute_manage_fork_attack(struct brute_stats *stats, u64 now)
> +{
> +	u64 last_fork_crash;
> +
> +	last_fork_crash = brute_update_crash_period(stats, now);
> +	if (brute_attack_running(stats))
> +		print_fork_attack_running();
> +
> +	return last_fork_crash;
> +}
> +
> +/**
> + * brute_get_exec_stats() - Get the exec statistics.
> + * @stats: When this function is called, this parameter must point to the
> + *         current process' statistical data. When this function returns, this
> + *         parameter points to the parent process' statistics of the fork
> + *         hierarchy that hold the current process' statistics.
> + *
> + * To manage a brute force attack that happens through the execve system call it
> + * is not possible to use the statistical data hold by this process due to these
> + * statistics disappear when this task is finished. In this scenario this data
> + * should be tracked by the statistics of a higher fork hierarchy (the hierarchy
> + * that contains the process that forks before the execve system call).
> + *
> + * To find these statistics the current fork hierarchy must be traversed up
> + * until new statistics are found.
> + *
> + * Context: Must be called with tasklist_lock and brute_stats_ptr_lock held.
> + */
> +static void brute_get_exec_stats(struct brute_stats **stats)
> +{
> +	const struct task_struct *task = current;
> +	struct brute_stats **p_stats;
> +
> +	do {
> +		if (!task->real_parent) {
> +			*stats = NULL;
> +			return;
> +		}
> +
> +		p_stats = brute_stats_ptr(task->real_parent);
> +		task = task->real_parent;
> +	} while (*stats == *p_stats);
> +
> +	*stats = *p_stats;
> +}

See Yama's task_is_descendant() for how to walk up the process tree
(and I think the process group stuff will save some steps too); you
don't need tasklist_lock held, just rcu_read_lock held, AIUI:
Documentation/RCU/listRCU.rst

And since you're passing this stats struct back up, and it would be outside of rcu read lock, you'd want to do a "get" on it first:

	rcu_read_lock();
	loop {
		...
	}
	refcount_inc_not_zero(&(*p_stats)->refc);
	rcu_read_unlock();

	*stats = *p_stats



> +
> +/**
> + * brute_update_exec_crash_period() - Update the exec crash period.
> + * @stats: When this function is called, this parameter must point to the
> + *         current process' statistical data. When this function returns, this
> + *         parameter points to the updated statistics (statistics that track the
> + *         info to manage a brute force attack that happens through the execve
> + *         system call).
> + * @now: The current timestamp in jiffies.
> + * @last_fork_crash: The last fork crash timestamp before updating it.
> + *
> + * If this is the first update of the statistics used to manage a brute force
> + * attack that happens through the execve system call, its last crash timestamp
> + * (the timestamp that shows when the execve was called) cannot be used to
> + * compute the crash period's EMA. Instead, the last fork crash timestamp should
> + * be used (the last crash timestamp of the child fork hierarchy before updating
> + * the crash period). This allows that in a brute force attack that happens
> + * through the fork system call, the exec and fork statistics are the same. In
> + * this situation, the mitigation method will act only in the processes that are
> + * sharing the fork statistics. This way, the process that forked before the
> + * execve system call will not be involved in the mitigation method. In this
> + * scenario, the parent is not responsible of the child's behaviour.
> + *
> + * 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.
> + * Return: -EFAULT if there are no exec statistics. Zero otherwise.
> + */
> +static int brute_update_exec_crash_period(struct brute_stats **stats,
> +					  u64 now, u64 last_fork_crash)
> +{
> +	brute_get_exec_stats(stats);
> +	if (!*stats)
> +		return -EFAULT;

This isn't EFAULT (userspace memory fault), but rather more EINVAL or
ESRCH.

> +
> +	spin_lock(&(*stats)->lock);
> +	if (!(*stats)->faults)
> +		(*stats)->jiffies = last_fork_crash;
> +	spin_unlock(&(*stats)->lock);
> +
> +	brute_update_crash_period(*stats, now);

and then you can add:

	if (refcount_dec_and_test(&(*stats)->refc))
		kfree(*stats);

(or better yet, make that a helper) named something like
"put_brute_stats".

> +	return 0;
> +}

I find the re-writing of **stats confusing here -- I think you should
leave that unmodified, and instead return a pointer (instead of "int"),
and for errors, use ERR_PTR(-ESRCH)

> +
> +/**
> + * brute_get_crash_period() - Get the application crash period.
> + * @stats: Statistical data shared by all the fork hierarchy processes.
> + *
> + * 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 brute_stats_ptr_lock
> + *          held.
> + * Return: The application crash period.
> + */
> +static u64 brute_get_crash_period(struct brute_stats *stats)
> +{
> +	u64 crash_period;
> +
> +	spin_lock(&stats->lock);
> +	crash_period = stats->period;
> +	spin_unlock(&stats->lock);
> +
> +	return crash_period;
> +}

return READ_ONCE(stats->period);

> +
> +/**
> + * print_exec_attack_running() - Warn about an exec brute force attack.
> + * @stats: Statistical data shared by all the fork hierarchy processes.
> + *
> + * The statistical data shared by all the fork hierarchy processes cannot be
> + * NULL.
> + *
> + * Before showing the process name it is mandatory to find a process that holds
> + * a pointer to the exec statistics.
> + *
> + * Context: Must be called with tasklist_lock and brute_stats_ptr_lock held.
> + */
> +static void print_exec_attack_running(const struct brute_stats *stats)
> +{
> +	struct task_struct *p;
> +	struct brute_stats **p_stats;
> +	bool found = false;
> +
> +	for_each_process(p) {
> +		p_stats = brute_stats_ptr(p);
> +		if (*p_stats == stats) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (WARN(!found, "No exec process\n"))
> +		return;
> +
> +	pr_warn("Exec brute force attack detected [%s]\n", p->comm);
> +}

Same logic to change here as above for talking the process list. (IIUC, since
you're only reading, you don't need tasklist_lock, just rcu_read_lock.)
But, if I'm reading this right, you only ever call this with "current".
It seems like it would be way more efficient to just use "current"
instead?

> +
> +/**
> + * brute_manage_exec_attack() - Manage an exec brute force attack.
> + * @stats: Statistical data shared by all the fork hierarchy processes.
> + * @now: The current timestamp in jiffies.
> + * @last_fork_crash: The last fork crash timestamp before updating it.
> + *
> + * For a correct management of an exec brute force attack it is only necessary
> + * to update the exec statistics and test if an attack is happening based on
> + * these data.
> + *
> + * It is important to note that if the fork and exec crash periods are the same,
> + * the attack test is avoided. This allows that in a brute force attack that
> + * happens through the fork system call, the mitigation method does not act on
> + * the parent process of the fork hierarchy.
> + *
> + * 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_manage_exec_attack(struct brute_stats *stats, u64 now,
> +				     u64 last_fork_crash)
> +{
> +	int ret;
> +	struct brute_stats *exec_stats = stats;
> +	u64 fork_period;
> +	u64 exec_period;
> +
> +	ret = brute_update_exec_crash_period(&exec_stats, now, last_fork_crash);
> +	if (WARN(ret, "No exec statistical data\n"))
> +		return;

I think this should fail closed: if there's a static processing error,
treat it as an attack.

> +
> +	fork_period = brute_get_crash_period(stats);
> +	exec_period = brute_get_crash_period(exec_stats);
> +	if (fork_period == exec_period)
> +		return;
> +
> +	if (brute_attack_running(exec_stats))
> +		print_exec_attack_running(exec_stats);
> +}
> +
> +/**
> + * brute_task_fatal_signal() - Target for the task_fatal_signal hook.
> + * @siginfo: Contains the signal information.
> + *
> + * To detect a brute force attack is necessary to update the fork and exec
> + * statistics in every fatal crash and act based on these data.
> + *
> + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> + * and brute_stats::lock since the task_free hook can be called from an IRQ
> + * context during the execution of the task_fatal_signal hook.
> + */
> +static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo)
> +{
> +	struct brute_stats **stats;
> +	unsigned long flags;
> +	u64 last_fork_crash;
> +	u64 now = get_jiffies_64();
> +
> +	stats = brute_stats_ptr(current);
> +	read_lock(&tasklist_lock);
> +	read_lock_irqsave(&brute_stats_ptr_lock, flags);
> +
> +	if (WARN(!*stats, "No statistical data\n")) {
> +		read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> +		read_unlock(&tasklist_lock);
> +		return;
>  	}
> +
> +	last_fork_crash = brute_manage_fork_attack(*stats, now);
> +	brute_manage_exec_attack(*stats, now, last_fork_crash);
> +	read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> +	read_unlock(&tasklist_lock);
>  }
> 
>  /*
> @@ -235,6 +694,7 @@ static struct security_hook_list brute_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(task_alloc, brute_task_alloc),
>  	LSM_HOOK_INIT(bprm_committing_creds, brute_task_execve),
>  	LSM_HOOK_INIT(task_free, brute_task_free),
> +	LSM_HOOK_INIT(task_fatal_signal, brute_task_fatal_signal),
>  };
> 
>  /**
> --
> 2.25.1
> 

I think this is very close!

-- 
Kees Cook

Powered by blists - more mailing lists