On Sun, Mar 07, 2021 at 12:30:25PM +0100, John Wood wrote:

Add a new Kconfig file to define a menu entry under "Security options" to enable the "Fork brute force attack detection and mitigation" feature.

For a correct management of a fork brute force attack it is necessary that all the tasks hold statistical data. The same statistical data needs to be shared between all the tasks that hold the same memory contents or in other words, between all the tasks that have been forked without any execve call. So, define a statistical data structure to hold all the necessary information shared by all the fork hierarchy processes. This info is basically the number of crashes, the last crash timestamp and the crash period's moving average.

When a forked task calls the execve system call, the memory contents are set with new values. So, in this scenario the parent's statistical data no need to be shared. Instead, a new statistical data structure must be allocated to start a new hierarchy.

The statistical data that is shared between all the fork hierarchy processes needs to be freed when this hierarchy disappears.

So, based in all the previous information define a LSM with three hooks to manage all the commented cases. These hooks are "task_alloc" to do the fork management, "bprm_committing_creds" to do the execve management and "task_free" to release the resources.

Also, add to the task_struct's security blob the pointer to the statistical data. This way, all the tasks will have access to this information.

Signed-off-by: John Wood john.wood@gmx.com

security/Kconfig | 11 +- security/Makefile | 4 + security/brute/Kconfig | 12 ++ security/brute/Makefile | 2 + security/brute/brute.c | 257 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 281 insertions(+), 5 deletions(-) create mode 100644 security/brute/Kconfig create mode 100644 security/brute/Makefile create mode 100644 security/brute/brute.c

diff --git a/security/Kconfig b/security/Kconfig index 7561f6f99f1d..204bb311b1f1 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -240,6 +240,7 @@ source "security/safesetid/Kconfig" source "security/lockdown/Kconfig"

source "security/integrity/Kconfig" +source "security/brute/Kconfig"

choice prompt "First legacy 'major LSM' to be initialized" @@ -277,11 +278,11 @@ endchoice

config LSM string "Ordered list of enabled LSMs"

• default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
• default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
• default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
• default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
• default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"

• default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
• default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
• default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
• default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
• default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"

It probably doesn't matter much, but I think brute should be added between lockdown and yama.

help A comma-separated list of LSMs, in initialization order. Any LSMs left off this list will be ignored. This can be diff --git a/security/Makefile b/security/Makefile index 3baf435de541..1236864876da 100644 --- a/security/Makefile +++ b/security/Makefile @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ # Object integrity file lists subdir-$(CONFIG_INTEGRITY) += integrity obj-$(CONFIG_INTEGRITY) += integrity/

+# Object brute file lists +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/

I don't think subdir is needed here? I think you can use obj-... like loadpin, etc.

diff --git a/security/brute/Kconfig b/security/brute/Kconfig new file mode 100644 index 000000000000..1bd2df1e2dec --- /dev/null +++ b/security/brute/Kconfig @@ -0,0 +1,12 @@ +# SPDX-License-Identifier: GPL-2.0 +config SECURITY_FORK_BRUTE

• bool "Fork brute force attack detection and mitigation"
• depends on SECURITY
• help

•  This is an LSM that stops any fork brute force attack against
  

•  vulnerable userspace processes. The detection method is based on
  

•  the application crash period and as a mitigation procedure all the
  

•  offending tasks are killed. Like capabilities, this security module
  

•  stacks with other LSMs.
  

I'm not sure the stacking needs mentioning, but okay. :)

•  If you are unsure how to answer this question, answer N.
  

diff --git a/security/brute/Makefile b/security/brute/Makefile new file mode 100644 index 000000000000..d3f233a132a9 --- /dev/null +++ b/security/brute/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute.o diff --git a/security/brute/brute.c b/security/brute/brute.c new file mode 100644 index 000000000000..99d099e45112 --- /dev/null +++ b/security/brute/brute.c @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: GPL-2.0

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <asm/current.h>

Why is this needed?

+#include <linux/bug.h> +#include <linux/compiler.h> +#include <linux/errno.h> +#include <linux/gfp.h> +#include <linux/init.h> +#include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/lsm_hooks.h> +#include <linux/printk.h> +#include <linux/refcount.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h>

+/**

• • struct brute_stats - Fork brute force attack statistics.
• • @lock: Lock to protect the brute_stats structure.
• • @refc: Reference counter.
• • @faults: Number of crashes.
• • @jiffies: Last crash timestamp.
• • @period: Crash period's moving average.
• • This structure holds the statistical data shared by all the fork hierarchy
• • processes.
• */

+struct brute_stats {

• spinlock_t lock;
• refcount_t refc;
• unsigned char faults;
• u64 jiffies;
• u64 period;

+};

I assume the max-255 "faults" will be explained... why is this so small?

+/*

• • brute_blob_sizes - LSM blob sizes.
• • To share statistical data among all the fork hierarchy processes, define a
• • pointer to the brute_stats structure as a part of the task_struct's security
• • blob.
• */

+static struct lsm_blob_sizes brute_blob_sizes __lsm_ro_after_init = {

• .lbs_task = sizeof(struct brute_stats *),

+};

+/**

• • brute_stats_ptr() - Get the pointer to the brute_stats structure.
• • @task: Task that holds the statistical data.
• • Return: A pointer to a pointer to the brute_stats structure.
• */

+static inline struct brute_stats **brute_stats_ptr(struct task_struct *task) +{

• return task->security + brute_blob_sizes.lbs_task;

+}

+/**

• • brute_new_stats() - Allocate a new statistics structure.
• • If the allocation is successful the reference counter is set to one to
• • indicate that there will be one task that points to this structure. Also, the
• • last crash timestamp is set to now. This way, it is possible to compute the
• • application crash period at the first fault.
• • Return: NULL if the allocation fails. A pointer to the new allocated

• •     statistics structure if it success.
    

• */

+static struct brute_stats *brute_new_stats(void) +{

• struct brute_stats *stats;
• stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
• if (!stats)

• return NULL;
  

Since this is tied to process creation, I think it might make sense to have a dedicated kmem cache for this (instead of using the "generic" kmalloc). See kmem_cache_{create,*alloc,free}

• spin_lock_init(&stats->lock);
• refcount_set(&stats->refc, 1);
• stats->faults = 0;
• stats->jiffies = get_jiffies_64();
• stats->period = 0;

And either way, I'd recommend using the "z" variant of the allocator (kmem_cache_zalloc, kzalloc) to pre-zero everything (and then you can drop the "= 0" lines here).

• return stats;

+}

+/**

• • brute_share_stats() - Share the statistical data between processes.
• • @src: Source of statistics to be shared.
• • @dst: Destination of statistics to be shared.
• • Copy the src's pointer to the statistical data structure to the dst's pointer
• • to the same structure. Since there is a new process that shares the same
• • data, increase the reference counter. The src's pointer 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_alloc hook.
• */

+static void brute_share_stats(struct brute_stats *src,

• 	      struct brute_stats **dst)
  

+{

• unsigned long flags;
• spin_lock_irqsave(&src->lock, flags);
• refcount_inc(&src->refc);
• *dst = src;
• spin_unlock_irqrestore(&src->lock, flags);

+}

+/**

• • brute_task_alloc() - Target for the task_alloc hook.
• • @task: Task being allocated.
• • @clone_flags: Contains the flags indicating what should be shared.
• • For a correct management of a fork brute force attack it is necessary that
• • all the tasks hold statistical data. The same statistical data needs to be
• • shared between all the tasks that hold the same memory contents or in other
• • words, between all the tasks that have been forked without any execve call.
• • To ensure this, if the current task doesn't have statistical data when forks,
• • it is mandatory to allocate a new statistics structure and share it between
• • this task and the new one being allocated. Otherwise, share the statistics
• • that the current task already has.
• • 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;
• stats = brute_stats_ptr(task);
• p_stats = brute_stats_ptr(current);
• if (likely(*p_stats)) {

• brute_share_stats(*p_stats, stats);
  

• return 0;
  

• }
• *stats = brute_new_stats();
• if (!*stats)

• return -ENOMEM;
  

• brute_share_stats(*stats, p_stats);
• return 0;

+}

During the task_alloc hook, aren't both "current" and "task" already immutable (in the sense that no lock needs to be held for brute_share_stats())?

And what is the case where brute_stats_ptr(current) returns NULL?

+/**

• • brute_task_execve() - Target for the bprm_committing_creds hook.
• • @bprm: Points to the linux_binprm structure.
• • When a forked task calls the execve system call, the memory contents are set
• • with new values. So, in this scenario the parent's statistical data no need
• • to be shared. Instead, a new statistical data structure must be allocated to
• • start a new hierarchy. This condition is detected when the statistics
• • reference counter holds a value greater than or equal to two (a fork always
• • sets the statistics reference counter to a minimum of two since the parent
• • and the child task are sharing the same data).
• • However, if the execve function is called immediately after another execve
• • call, althought the memory contents are reset, there is no need to allocate
• • a new statistical data structure. This is possible because at this moment
• • 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.
• */

+static void brute_task_execve(struct linux_binprm *bprm) +{

• struct brute_stats **stats;
• unsigned long flags;
• stats = brute_stats_ptr(current);
• if (WARN(!*stats, "No statistical data\n"))

• return;
  

• spin_lock_irqsave(&(*stats)->lock, flags);
• 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);
  

• return;
  

• }
• /* execve call after a fork call */
• spin_unlock_irqrestore(&(*stats)->lock, flags);
• *stats = brute_new_stats();
• WARN(!*stats, "Cannot allocate statistical data\n");

+}

I don't think any of this locking is needed -- you're always operating on "current", so its brute_stats will always be valid.

+/**

• • brute_task_free() - Target for the task_free hook.
• • @task: Task about to be freed.
• • 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.
• */

+static void brute_task_free(struct task_struct *task) +{

• struct brute_stats **stats;
• unsigned long flags;
• bool refc_is_zero;
• stats = brute_stats_ptr(task);
• if (WARN(!*stats, "No statistical data\n"))

• return;
  

• spin_lock_irqsave(&(*stats)->lock, flags);
• refc_is_zero = refcount_dec_and_test(&(*stats)->refc);
• spin_unlock_irqrestore(&(*stats)->lock, flags);
• if (refc_is_zero) {

• kfree(*stats);
  

• *stats = NULL;
  

• }

+}

Same thing -- this is what dec_and_test is for: it's atomic, so no locking needed.

+/*

• • brute_hooks - Targets for the LSM's hooks.
• */

+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),

+};

+/**

• • brute_init() - Initialize the brute LSM.
• • Return: Always returns zero.
• */

+static int __init brute_init(void) +{

• pr_info("Brute initialized\n");
• security_add_hooks(brute_hooks, ARRAY_SIZE(brute_hooks),

• 	   KBUILD_MODNAME);
  

• return 0;

+}

+DEFINE_LSM(brute) = {

• .name = KBUILD_MODNAME,
• .init = brute_init,
• .blobs = &brute_blob_sizes,

+};

2.25.1

On Sat, Mar 20, 2021 at 04:01:53PM +0100, John Wood wrote:

Hi, First of all thanks for the review. More info and questions inline.

On Wed, Mar 17, 2021 at 07:00:56PM -0700, Kees Cook wrote:

...

On Sun, Mar 07, 2021 at 12:30:25PM +0100, John Wood wrote:

...

config LSM string "Ordered list of enabled LSMs"

• default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
• default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
• default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
• default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
• default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"

• default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
• default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
• default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
• default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
• default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"

It probably doesn't matter much, but I think brute should be added between lockdown and yama.

What is the rationale for the stacking order (in relation with brute and lockdown)?

lockdown has some very early hooks, so leaving it at the front seems organizationally correct to me. It doesn't really matter, though, so perhaps we should just alphabetize them, but that's for another day.

...

...

diff --git a/security/Makefile b/security/Makefile index 3baf435de541..1236864876da 100644 --- a/security/Makefile +++ b/security/Makefile @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ # Object integrity file lists subdir-$(CONFIG_INTEGRITY) += integrity obj-$(CONFIG_INTEGRITY) += integrity/

+# Object brute file lists +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/

I don't think subdir is needed here? I think you can use obj-... like loadpin, etc.

loadpin also uses subdir just like selinux, smack, tomoyo, etc.. So, why is it not necessary for brute?

Oops, yes, my mistake. I didn't look at the Makefile as a whole. I will adjust my suggestion as: please split subdir and obj as done by the other LSMs (integrity should be fixed to do the same, but that doesn't need to be part of this series).

...

...

+#include <asm/current.h>

Why is this needed?

IIUC, the "current" macro is defined in this header. I try to include the appropiate header for every macro and function used.

The common approach is actually to minimize the number of explicit headers so that if header files includes need to be changed, they only need to be changed internally instead of everywhere in the kernel. Please find an appropriately minimal set of headers to include.

...

...

+/**

• • struct brute_stats - Fork brute force attack statistics.
• • @lock: Lock to protect the brute_stats structure.
• • @refc: Reference counter.
• • @faults: Number of crashes.
• • @jiffies: Last crash timestamp.
• • @period: Crash period's moving average.
• • This structure holds the statistical data shared by all the fork hierarchy
• • processes.
• */

+struct brute_stats {

• spinlock_t lock;
• refcount_t refc;
• unsigned char faults;
• u64 jiffies;
• u64 period;

+};

I assume the max-255 "faults" will be explained... why is this so small?

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. In this case, the BRUTE_MAX_FAULTS is defined as 200.

Okay, so given the choise of BRUTE_MAX_FAULTS, you limited the storage size? I guess I worry about this somehow wrapping around easily. Given the struct has padding due to the u8 storage, it seems like just using int would be fine too.

...

...

+static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) +{

• struct brute_stats **stats, **p_stats;
• stats = brute_stats_ptr(task);
• p_stats = brute_stats_ptr(current);
• if (likely(*p_stats)) {

• brute_share_stats(*p_stats, stats);
  

• return 0;
  

• }
• *stats = brute_new_stats();
• if (!*stats)

• return -ENOMEM;
  

• brute_share_stats(*stats, p_stats);
• return 0;

+}

During the task_alloc hook, aren't both "current" and "task" already immutable (in the sense that no lock needs to be held for brute_share_stats())?

I will work on it.

...

And what is the case where brute_stats_ptr(current) returns NULL?

Sorry, but I don't understand what you are trying to explain me. brute_stats_ptr(current) returns a pointer to a pointer. So, I think your question is: What's the purpose of the "if (likely(*p_stats))" check? If it is the case, this check is to guarantee that all the tasks have statistical data. If some task has been allocated prior the brute LSM initialization, this task doesn't have stats. So, with this check all the tasks that fork have stats.

Thank you for figuring out my poorly-worded question. :) Yes, I was curious about the "if (likely(*p_stats))". It seems like it shouldn't be possible for a process to lack a stats allocation: the LSMs get initialized before processes. If you wanted to be defensive, I would have expected:

if (WARN_ON_ONCE(!*p_stats)) return -ENOMEM;

or something (brute should be able to count on the kernel internals behaving here: you're not expecting any path where this could happen).

...

...

+/**

• • brute_task_execve() - Target for the bprm_committing_creds hook.
• • @bprm: Points to the linux_binprm structure.
• • When a forked task calls the execve system call, the memory contents are set
• • with new values. So, in this scenario the parent's statistical data no need
• • to be shared. Instead, a new statistical data structure must be allocated to
• • start a new hierarchy. This condition is detected when the statistics
• • reference counter holds a value greater than or equal to two (a fork always
• • sets the statistics reference counter to a minimum of two since the parent
• • and the child task are sharing the same data).
• • However, if the execve function is called immediately after another execve
• • call, althought the memory contents are reset, there is no need to allocate
• • a new statistical data structure. This is possible because at this moment
• • 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.
• */

+static void brute_task_execve(struct linux_binprm *bprm) +{

• struct brute_stats **stats;
• unsigned long flags;
• stats = brute_stats_ptr(current);
• if (WARN(!*stats, "No statistical data\n"))

• return;
  

• spin_lock_irqsave(&(*stats)->lock, flags);
• 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);
  

• return;
  

• }
• /* execve call after a fork call */
• spin_unlock_irqrestore(&(*stats)->lock, flags);
• *stats = brute_new_stats();
• WARN(!*stats, "Cannot allocate statistical data\n");

+}

I don't think any of this locking is needed -- you're always operating on "current", so its brute_stats will always be valid.

But another process (that share the same stats) could be modifying this concurrently.

Scenario 1: cpu 1 writes stats and cpu 2 writes stats. Scenario 2: cpu 1 writes stats, then IRQ on the same cpu writes stats.

I think it is possible. So AFAIK we need locking. Sorry if I am wrong.

Maybe I'm misunderstanding, but even your comments on the function say that the zeroing path is there to avoid a new allocation, since only 1 thread has access to that "stats". (i.e. no locking needed), and in the other path, a new stats is allocated (no locking needed). What are the kernel execution paths you see where you'd need locking here?

...

...

+/**

• • brute_task_free() - Target for the task_free hook.
• • @task: Task about to be freed.
• • 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.
• */

+static void brute_task_free(struct task_struct *task) +{

• struct brute_stats **stats;
• unsigned long flags;
• bool refc_is_zero;
• stats = brute_stats_ptr(task);
• if (WARN(!*stats, "No statistical data\n"))

• return;
  

• spin_lock_irqsave(&(*stats)->lock, flags);
• refc_is_zero = refcount_dec_and_test(&(*stats)->refc);
• spin_unlock_irqrestore(&(*stats)->lock, flags);
• if (refc_is_zero) {

• kfree(*stats);
  

• *stats = NULL;
  

• }

+}

Same thing -- this is what dec_and_test is for: it's atomic, so no locking needed.

Ok, in this case I can see that the locking is not necessary due to the stats::refc is atomic. But in the previous case, faults, jiffies and period are not atomic. So I think the lock is necessary. If not, what am I missing?

I thought the code had established that there could only be a single stats holder for that code, so no locking. Maybe I misunderstood?

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@gmx.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!

On Sat, Mar 20, 2021 at 04:34:06PM +0100, John Wood wrote:

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:

...

@@ -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.

To be coherent in the previous patch. In the previous patch the kmalloc could use GFP_KERNEL due to the call was made out of an atomic context. Now, with the new lock it needs GFP_ATOMIC. So the question:

If finally it need to use GFP_ATOMIC, the first patch need to use it even if it is not necessary?

It's probably not a big deal, but for me, I'd just do GFP_ATOMIC from the start, maybe add a comment that says "some LSM hooks are from atomic context" or something.

...

...

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.

In this case I think this is possible:

Scenario 1: cpu 1 writes the stats pointer and cpu 2 is navigating the processes tree reading the same stats pointer.

Scenario 2: cpu 1 is navigating the processes tree reading the stats pointer and in IRQ the same stats pointer is wrote.

So, we need locking. Am I wrong?

But only the refcount is being incremented, yes? That doesn't need a lock because it's already an atomic.

...

...

/** @@ -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. :)

So, IIUC I need to introduce all the locks in the initial patch even if they are not necessary. Am I right?

I would find it easier to follow. Perhaps other reviewers would have a different opinion.

...

...

@@ -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.

Here I can see the same two scenarios noted before. So I think the locking is needed. Am I right?

...

...

/** @@ -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:

No comment. I think I am missing something. I need to clarify the previous cases before to work on the next ones. Sorry and thanks for the guidance.

Right -- so, there are a few concurrency cases you need to worry about, AIUI:

1- stats lifetime (based on creation/death of tasks) 2- stats value being written vs read 3- stats values being written/read vs stats lifetime

Using refcount_t in the standard pattern (as you're doing) should entirely cover "1".

Since the values read from stats are mostly independent, it should be possible to use READ_ONCE() in the readers and WRITE_ONCE() under a lock in the writers (this is case "2").

For "3", I think the implicit locking of "current" keeps you safe (as in, the stats can't go away because "current" will always have a reference on it).

I see two places where stats are written. One appears to be the brute_task_execve() case where only 1 thread exists, so there's no lock needed, and the other case is brute_update_crash_period(), which makes sense to me to lock: two tasks might be sharing a stats as they crash.

Of course, I could easily be missing something here, but it looks like much less locking is needed.

...

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)?

No problem. I think this is easier than locking :)

Heh, for the most part, yes. ;) Though I have my own nightmares[1] about sysfs.

...

...

+/**

• • 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).

Thanks for the refactory. I will work on it (if I can understand locking). :(

It may be worth reading Documentation/memory-barriers.txt which has some more details.

...

...

+/**

• • 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

Thanks for the suggestions. I will work on it for the next version. Anyway, in the first version Kees Cook and Jann Horn noted that some tasks could escape the rcu read lock and that alternate locking were needed.

Extract from the RFC:

[Kees Cook] 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.

[Jann Horn] Good point; the proper way to deal with this would probably be to take the tasklist_lock in read mode around this loop (with read_lock(&tasklist_lock) / read_unlock(&tasklist_lock)), which pairs with the write_lock_irq(&tasklist_lock) in copy_process(). Thanks to the fatal_signal_pending() check while holding the lock in copy_process(), that would be race-free - any fork() that has not yet inserted the new task into the global task list would wait for us to drop the tasklist_lock, then bail out at the fatal_signal_pending() check.

I think that this scenario is still possible. So the tasklist_lock is necessary. Am I right?

Oops, yeah, best to listen to Jann and past-me. :) Were these comments about finding the parent or killing offenders?

...

...

• 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".

Sorry, but I don't understand why we need to free the stats here. What is the rationale behind this change?

Err, I think I may have quoted the wrong chunk of your patch! Sorry; I was talking about the place where you did a free, I think? Disregard this for now. :)

...

...

+/**

• • 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.

Do you mean to trigger the mitigation of a brute force attack over this task? So, IIUC you suggest that instead of generate warnings if there isn't statistical data, we need to trigger the mitigation? This can be applied to the case where the allocation of a brute_stats structure fails?

Right -- it should be an impossible scenario that the stats are _missing_. There is not an expected execution path in the kernel where that could happen, so if you're testing for it (and correctly generating a WARN), it should _also_ fail closed: an impossible case has been found, so assume userspace is under attack. (Otherwise it could serve as a bypass for an attacker who has found a way to navigate a process into this state.)

-Kees

On Wed, Mar 17, 2021 at 09:00:51PM -0700, Kees Cook wrote:

On Sun, Mar 07, 2021 at 12:30:27PM +0100, John Wood wrote:

...

#include <asm/current.h> +#include <asm/rwonce.h> +#include <asm/siginfo.h> +#include <asm/signal.h> +#include <linux/binfmts.h> #include <linux/bug.h> #include <linux/compiler.h> +#include <linux/cred.h> +#include <linux/dcache.h> #include <linux/errno.h> +#include <linux/fs.h> #include <linux/gfp.h> +#include <linux/if.h> #include <linux/init.h> #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/lsm_hooks.h> #include <linux/math64.h> +#include <linux/netdevice.h> +#include <linux/path.h> #include <linux/printk.h> #include <linux/refcount.h> #include <linux/rwlock.h> @@ -19,9 +29,35 @@ #include <linux/sched.h> #include <linux/sched/signal.h> #include <linux/sched/task.h> +#include <linux/signal.h> +#include <linux/skbuff.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/stat.h> #include <linux/types.h> +#include <linux/uidgid.h>

This is really a LOT of includes. Are you sure all of these are explicitly needed?

I try to add the needed header for every macro and function used. If there is a better method to do it I will apply it. Thanks.

...

/**

• struct brute_stats - Fork brute force attack statistics.

@@ -30,6 +66,9 @@

• @faults: Number of crashes.
• @jiffies: Last crash timestamp.
• @period: Crash period's moving average.

• • @saved_cred: Saved credentials.
• • @network: Network activity flag.
• • @bounds_crossed: Privilege bounds crossed flag.
  • This structure holds the statistical data shared by all the fork hierarchy
  • processes.

@@ -40,6 +79,9 @@ struct brute_stats { unsigned char faults; u64 jiffies; u64 period;

• struct brute_cred saved_cred;
• unsigned char network : 1;
• unsigned char bounds_crossed : 1;

If you really want to keep faults a "char", I would move these bools after "faults" to avoid adding more padding.

Understood. Thanks.

...

+/**

• • brute_is_setid() - Test if the executable file has the setid flags set.
• • @bprm: Points to the linux_binprm structure.
• • Return: True if the executable file has the setid flags set. False otherwise.
• */

+static bool brute_is_setid(const struct linux_binprm *bprm) +{

• struct file *file = bprm->file;
• struct inode *inode;
• umode_t mode;
• if (!file)

• return false;
  

• inode = file->f_path.dentry->d_inode;
• mode = inode->i_mode;
• return !!(mode & (S_ISUID | S_ISGID));

+}

Oh, er, no, this should not reinvent the wheel. You just want to know if creds got elevated, so you want bprm->secureexec; this gets correctly checked in cap_bprm_creds_from_file().

Ok, I will work on it for the next version.

...

+/**

• • brute_reset_stats() - Reset the statistical data.
• • @stats: Statistics to be reset.
• • @is_setid: The executable file has the setid flags set.
• • Reset the faults and period and set the last crash timestamp to now. This
• • way, it is possible to compute the application crash period at the next
• • fault. Also, save the credentials of the current task and update the
• • bounds_crossed flag based on a previous network activity and the is_setid
• • parameter.
• • The statistics to be reset cannot be NULL.
• • Context: Must be called with interrupts disabled and brute_stats_ptr_lock

• •      and brute_stats::lock held.
    

• */

+static void brute_reset_stats(struct brute_stats *stats, bool is_setid) +{

• const struct cred *cred = current_cred();
• stats->faults = 0;
• stats->jiffies = get_jiffies_64();
• stats->period = 0;
• stats->saved_cred.uid = cred->uid;
• stats->saved_cred.gid = cred->gid;
• stats->saved_cred.suid = cred->suid;
• stats->saved_cred.sgid = cred->sgid;
• stats->saved_cred.euid = cred->euid;
• stats->saved_cred.egid = cred->egid;
• stats->saved_cred.fsuid = cred->fsuid;
• stats->saved_cred.fsgid = cred->fsgid;
• stats->bounds_crossed = stats->network || is_setid;

+}

I would include brute_reset_stats() in the first patch (and add to it as needed). To that end, it can start with a memset(stats, 0, sizeof(*stats));

So, need all the struct fields to be introduced in the initial patch? Even if they are not needed in the initial patch? I'm confused.

...

+/**

• • brute_priv_have_changed() - Test if the privileges have changed.
• • @stats: Statistics that hold the saved credentials.
• • The privileges have changed if the credentials of the current task are
• • different from the credentials saved in the statistics structure.
• • The statistics that hold the saved credentials cannot be NULL.
• • Context: Must be called with interrupts disabled and brute_stats_ptr_lock

• •      and brute_stats::lock held.
    

• • Return: True if the privileges have changed. False otherwise.
• */

+static bool brute_priv_have_changed(struct brute_stats *stats) +{

• const struct cred *cred = current_cred();
• bool priv_have_changed;
• priv_have_changed = !uid_eq(stats->saved_cred.uid, cred->uid) ||

• !gid_eq(stats->saved_cred.gid, cred->gid) ||
  

• !uid_eq(stats->saved_cred.suid, cred->suid) ||
  

• !gid_eq(stats->saved_cred.sgid, cred->sgid) ||
  

• !uid_eq(stats->saved_cred.euid, cred->euid) ||
  

• !gid_eq(stats->saved_cred.egid, cred->egid) ||
  

• !uid_eq(stats->saved_cred.fsuid, cred->fsuid) ||
  

• !gid_eq(stats->saved_cred.fsgid, cred->fsgid);
  

• return priv_have_changed;

+}

This should just be checked from bprm->secureexec, which is valid by the time you get to the bprm_committing_creds hook. You can just save the value to your stats struct instead of re-interrogating current_cred, etc.

Ok. Thanks.

...

+/**

• • brute_threat_model_supported() - Test if the threat model is supported.
• • @siginfo: Contains the signal information.
• • @stats: Statistical data shared by all the fork hierarchy processes.
• • To avoid false positives during the attack detection it is necessary to
• • narrow the possible cases. Only the following scenarios are taken into
• • account:
• • 1.- Launching (fork()/exec()) a setuid/setgid process repeatedly until a

• • desirable memory layout is got (e.g. Stack Clash).
    

• • 2.- Connecting to an exec()ing network daemon (e.g. xinetd) repeatedly until

• • a desirable memory layout is got (e.g. what CTFs do for simple network
    

• • service).
    

• • 3.- Launching processes without exec() (e.g. Android Zygote) and exposing

• • state to attack a sibling.
    

• • 4.- Connecting to a fork()ing network daemon (e.g. apache) repeatedly until

• • the previously shared memory layout of all the other children is exposed
    

• • (e.g. kind of related to HeartBleed).
    

• • In each case, a privilege boundary has been crossed:
• • Case 1: setuid/setgid process
• • Case 2: network to local
• • Case 3: privilege changes
• • Case 4: network to local
• • Also, only the signals delivered by the kernel are taken into account with
• • the exception of the SIGABRT signal since the latter is used by glibc for
• • stack canary, malloc, etc failures, which may indicate that a mitigation has
• • been triggered.
• • The signal information and 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: True if the threat model is supported. False otherwise.
• */

+static bool brute_threat_model_supported(const kernel_siginfo_t *siginfo,

• 			 struct brute_stats *stats)
  

+{

• bool bounds_crossed;
• if (siginfo->si_signo == SIGKILL && siginfo->si_code != SIGABRT)

• return false;
  

• spin_lock(&stats->lock);
• bounds_crossed = stats->bounds_crossed;
• bounds_crossed = bounds_crossed || brute_priv_have_changed(stats);
• stats->bounds_crossed = bounds_crossed;
• spin_unlock(&stats->lock);
• return bounds_crossed;

+}

I think this logic can be done with READ_ONCE()s and moved directly into brute_task_fatal_signal().

Thanks. I will work on locking.

...

+/**

• • brute_network() - Target for the socket_sock_rcv_skb hook.
• • @sk: Contains the sock (not socket) associated with the incoming sk_buff.
• • @skb: Contains the incoming network data.
• • A previous step to detect that a network to local boundary has been crossed
• • is to detect if there is network activity. To do this, it is only necessary
• • to check if there are data packets received from a network device other than
• • loopback.
• • 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 socket_sock_rcv_skb hook.
• • Return: -EFAULT if the current task doesn't have statistical data. Zero

• •     otherwise.
    

• */

+static int brute_network(struct sock *sk, struct sk_buff *skb) +{

• struct brute_stats **stats;
• unsigned long flags;
• if (!skb->dev || (skb->dev->flags & IFF_LOOPBACK))

• return 0;
  

• stats = brute_stats_ptr(current);

Uhh, is "current" valid here? I actually don't know this hook very well.

I think so, but I will try to study it. Thanks for noted this.

...

• read_lock_irqsave(&brute_stats_ptr_lock, flags);
• if (!*stats) {

• read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
  

• return -EFAULT;
  

• }
• spin_lock(&(*stats)->lock);
• (*stats)->network = true;
• spin_unlock(&(*stats)->lock);
• read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
• return 0;

+}

Thanks, John Wood