Read the "real_parent" field of task_struct

[John Wood]

Hi,

On Thu, Oct 01, 2020 at 08:29:58PM -0400, Valdis Klētnieks wrote:
> On Thu, 01 Oct 2020 19:49:02 +0200, John Wood said:
>
> > Idea: The purpose of my LSM is to detect and mitigate a fork brute force
> > attack. To do so, I need a hierarchy of fork processes. In other words, there
> > is a pointer in the task_struct structure that points to an statistics data
> > structure. This pointer is copied to the child process when a process forks.
> > This way, all the tasks that fork with the same root share the same statistics.
> > These statistics allow to compute the application crashing period and detect
> > the attack.
>
> How is this any better than applying a ulimit to the userid, and using the existing
> audit subsystem for reporting the attack, which is what that subsystem was
> designed for?

As far as I know, the ulimit allows to define limits (file size, max user
processes, open files, ...) but this is not what we want. We don't want to
limit any user resources. We need to detect if an application is crashing
quickly. More info can be found in this RFC:

https://lore.kernel.org/kernel-hardening/20200913072430.GA2965@ubuntu/T/

> > But there is the case when the task 0 is allocated. In this case it's not
> > possible to copy the parent pointer to the statistical data. The task 0 don't
> > have parent. In this scenario a new statistics structure need to be allocated.
>
> At that point, your LSM probably hasn't been initialized yet. If your LSM is being
> called before task 0 (let alone task 1) is created, there's probably something
> wonky going on.  Are you seeing this happen on an actual system?

The code shown in early mails has not been tested yet. I wanted to wait
until the access to the "real_parent" field of the task_struct structure was
clear to me. Anyway, good point. Thanks a lot.

I have changed the code to return an error if the allocated task don't have
parent. If the parent have statistics, this data is shared with the allocated
task. And if the parent don't have statistics, a new stats structure is
allocated for the allocated task and then shared with the parent.

static void brute_share_stats(struct brute_stats **src,
			      struct brute_stats **dst)
{
	spin_lock(&(*src)->lock);
	refcount_inc(&(*src)->refc);
	*dst = *src;
	spin_unlock(&(*src)->lock);
}

static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags)
{
	struct task_struct *p_task;
	struct brute_stats **stats, **p_stats;

	p_task = task->real_parent;		/////////// <----
	if (unlikely(!p_task))			/////////// <----
		return -ESRCH;

	stats = brute_stats_ptr(task);
	p_stats = brute_stats_ptr(p_task);	/////////// <----

	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;
}

This code is very untested. And now my first question: how can I read the
real_parent field in a secure way. Do I need to use an rcu_read_lock()/
rcu_read_unlock() block? Do I need to use rcu_dereference? Do I need to
use a read_lock(&task_list_lock)/read_unlock(&task_list_lock) block?

The lines with the mark are not clear to me. Sorry.

Thanks,
John Wood