Make criticall section, while working with task_struct, safe again.

[m1lua]

Description

This is second attempt to fix bug in exploit detection engine of LKRG.
First attempt: #339
Similar symptoms: #329

How This Could Be Reproduced?

To trigger the BUG the new process from kernel context could be spawned.
For example:

Set lkrg.block_modules=1 and and ensure, that some modules like netlink_diag af_packet_diag mptcp_diag unix_diag tcp_diag udp_diag raw_diag aren't taint in the kernel.

1st terminal: while true; do exec sh -c exit & done
2nd terminal: while true; do sudo ss -apn; done

second command will trigger kernel to spawn modprobe, but since lkrg is blocking modules - it will be rejected.
This way we can increase chance to win the race, when:

1. we increase task refs cont and call p_ed_is_off_off()
2. the task somehow enter in the destruction procedure
3. we compare corrupted cookies and hit if (unlikely(p_val != p_global_cnt_cookie))
4. the task now totally destructed
5. we calling p_ed_kill_task_by_task(p_source->p_ed_task.p_task); for uninitialized memory instead of alive task.

then in dmesg output you would see something like:

[ 4103.670400] LKRG: ALERT: DETECT: Task: 'off' flag corruption for pid 439, name sh
[ 4103.671411] LKRG: ALERT: BLOCK: Task: Killing pid 0, name 

and, possible, an Oops.

How Has This Been Tested?

After applying this patch - steps from above wouldn't trigger false-positives, and would not crash the kernel anymore.

At least on my setup.

[solardiz]

Thank you very much for trying to help figure this out and fix it! At least the reproduction steps should be useful to @Adam-pi3, I guess.

As to the patch in this PR, it looks wrong to me: you're removing required locking from p_security_bprm_committed_creds_ret (why?) and you're maybe papering over some other problem by holding the task's alloc_lock for the duration of p_ed_is_off_off. The latter lock may only matter if p_ed_is_off_off detects flag corruption and tries to kill the task, but such corruption (or false positive) is already a problem that we should figure out and avoid.

Of course, since we do have this task killing logic on corruption, we should also make this logic reliable (even if it's supposed to be unreachable in normal circumstances). So maybe this part of your patch has merit, but then perhaps it should be different. We don't currently know whether the spurious off flag corruption and the killing of wrong task via dangling pointer are parts of the same root cause or different - ideally, that's the first question we need answered to fix this issue.

My guess is that when you're running with these changes applied, you probably still do get the off flag corruption message occasionally, right?

[m1lua]

As to the patch in this PR, it looks wrong to me: you're removing required locking from p_security_bprm_committed_creds_ret (why?)

Because p_tasks_write_lock() will touch IRQ second time, first time it grabs in spin_lock_irqsave(&curr->alloc_lock, task_lock_flags);

The latter lock may only matter if p_ed_is_off_off detects flag corruption and tries to kill the task, but such corruption (or false positive) is already a problem that we should figure out and avoid.

May I ask, does there are any logic which unlink task_struct from candidates list in p_validate_task_f()?

I mean:

   get_task_struct(p_task);  // here we inc. ref. counter

   if ( (p_tmp = p_find_ed_by_pid(task_pid_nr(p_task))) == NULL) {
      // This process is not on the list!
      if (p_get_task_state(p_task) != TASK_DEAD) {
         p_ret = P_LKRG_GENERAL_ERROR;
         p_print_log(P_LOG_WATCH, "Can't find in internal tracking list pid %u, name %s", task_pid_nr(p_task), p_task->comm);
      }
      goto p_validate_task_out;
   }
   
   if (p_cmp_tasks(p_tmp, p_task, 1) > 0) { // and here the corruption take a place, right?
      // kill this process!
      p_ed_kill_task_by_task(p_task);  // here kern. Oopsing, because the pointer could became dangling
   }

p_validate_task_out:

   put_task_struct(p_task); // here we dec. ref. counter

So if the task somehow enter in the destruction procedure, it should be hocked, and jump out to p_validate_task_out, right?
So strong critical section should start near get_task_struct() and other side of it should be present in all task destructiors in the kernel?

killing of wrong task via dangling pointer are parts of the same root cause or different - ideally

+

My guess is that when you're running with these changes applied, you probably still do get the off flag corruption message occasionally, right?

Indeed, after some time

[53576.882071] LKRG: ALERT: DETECT: Task: 'off' flag corruption for pid 12627, name mksquashfs
[53576.882732] LKRG: ALERT: BLOCK: Task: Killing pid 12625, name modprobe

[Adam-pi3]

Hm... I tried to follow the steps which you described under Ubuntu 24.04 LTS, 6.8.0-36-generic in a VM (under VmWare) but it does not trigger any bug/problem and LKRG works as intended. I can see in dmesg logs that modules are blocked:

[Tue Jul  2 18:04:34 2024] LKRG: ALERT: BLOCK: Module: Loading of module name vsock_diag
[Tue Jul  2 18:04:34 2024] LKRG: ALERT: BLOCK: Module: Loading of module name xsk_diag
[Tue Jul  2 18:04:34 2024] LKRG: ALERT: BLOCK: Module: Loading of module name inet_diag
[Tue Jul  2 18:04:35 2024] LKRG: ALERT: BLOCK: Module: Loading of module name netlink_diag
[Tue Jul  2 18:04:35 2024] LKRG: ALERT: BLOCK: Module: Loading of module name af_packet_diag
[Tue Jul  2 18:04:35 2024] LKRG: ALERT: BLOCK: Module: Loading of module name unix_diag
[Tue Jul  2 18:04:35 2024] LKRG: ALERT: BLOCK: Module: Loading of module name inet_diag

However, no sign of any FP neither instability. What environment do you use to report the problem?

[m1lua]

However, no sign of any FP neither instability. What environment do you use to report the problem?

AMD EPYC 7261 8-Core Processor x2
Linux pve 6.10.0-rc4-secint+ #5 SMP PREEMPT_DYNAMIC Fri Jun 21 18:13:29 CEST 2024 x86_64 GNU/Linux
native server