Re: [RFC PATCH v3 4/6] security: kap: add the securityfs control plane

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [Medium] security: kap: dentry reference leak on child files in
  kapfs_mkdir()
- [Medium] security: kap: unattached kapfs_mnt resolves as root-relative
  in d_path()
- [Medium] security: kap: missing superblock ops and blocksize in
  kapfs_fill_super()
- [Medium] security: kap: data race on kap_task_state in
  kap_bind_pid_locked()
- [Medium] security: kap: race between kap_bind_pid_locked() and
  copy_process()
- [Medium] security: kap: kap_authority_prepare() bounds predicates by
  next kind not '}'
- [Low] security: kap: unsanitized lineage_root in kapfs_write()
  corrupts output
- [Low] security: kap: kapfs_write() silently succeeds on newline-only
  writes
- [Low] security: kap: unchecked u64 overflow in
  kap_parse_transition_sequence()
--

commit 9864f112a0785ec3a3604d782064cf536e36889c
Author: HARIHARA SUDHAN. R [off-list ref]

security: kap: add the securityfs control plane

This commit adds the KAP securityfs control plane at /sys/kernel/security/kap
and mounts an internal kapfs filesystem for managing igroups and their control
files.

[ ... ]

+static ssize_t kapfs_write(struct file *file, const char __user *buf, size_t len, loff_t *ppos)
+{

[ ... ]

[Severity: Low]
Should writing a newline-only string to a control file really succeed without
doing anything?

If a script writes an empty line (for example, echo "$PID" > kap.procs when
PID is unset), kapfs_write() returns len without validating the input or
updating any state, and this early return also prevents clearing
kap.lineage_root via a single-file write even though kap.install accepts an
empty lineage_root=.

[Severity: Low]
Can an unsanitized lineage_root corrupt kap.igfd and kap.events output?

KAP_FILE_LINEAGE_ROOT and kap_install_parse() only check the length of the
string, so embedded newlines break the single-line format of kap.igfd in
kapfs_show(), and quotes or backslashes are interpolated unescaped into the
"root" JSON field by kap_append_event_locked():

security/kap/igroup.c:kap_append_event_locked() {
    ...
	len = scnprintf(line, sizeof(line),
			"{\"ts_ns\":%llu,\"igroup\":\"%s\",\"decision\":\"%s\",\"operation\":\"%s\",\"target\":\"%s\",\"reason\":\"%s\",\"epoch\":%llu,\"root\":\"%s\",\"pid\":%d}\n",
			now, igroup->name, allowed ? "allow" : "deny",
			kap_hook_name(kind), escaped_target, reason, igroup->epoch,
			igroup->lineage_root, task_pid_nr(current));
    ...
}

[Severity: Medium]
Can kap_authority_prepare() bleed one predicate's allow or deny array into
the preceding predicate?

Both kap_validate_authority_payload() and kap_compile_kind_array() scan
backward from "kind": to '{' to find predicate_start, so keys appearing before
"kind" inside a predicate object are supported, but they set predicate_end to
the next "kind": rather than the closing '}' of the current predicate:

security/kap/igroup.c:kap_compile_kind_array() {
    ...
	while ((predicate = kap_next_predicate(predicate, end, kind_name))) {
		const char *next = strstr(predicate + 1, "\"kind\":");
		const char *predicate_start = predicate;
		const char *predicate_end = next && next < end ? next : end;
		int rc;

		while (predicate_start > auth && *predicate_start != '{')
			predicate_start--;
		rc = kap_compile_array(compiled, set, predicate_start,
				       predicate_end, array_name);
    ...
}

If one predicate omits "allow" or "deny" and the next predicate places its
"allow":[...] or "deny":[...] before its "kind": key, the slice
[predicate_start, predicate_end) spans across the boundary between the two
objects, causing validation to accept the malformed document and compiling the
second predicate's patterns into the first predicate's hook kind.

[Severity: Low]
Can kap_parse_transition_sequence() silently wrap on u64 overflow?

Unlike authority_epoch and expires_at, which use kstrtou64(),
kap_parse_transition_sequence() accumulates decimal digits without checking
for overflow:

security/kap/igroup.c:kap_parse_transition_sequence() {
    ...
	while (*cursor >= '0' && *cursor <= '9') {
		seen_digit = true;
		value = value * 10 + (*cursor - '0');
		cursor++;
	}
    ...
}

Writing a sequence number of 20 or more digits to kap.transition or
kap.install wraps modulo 2^64, which can corrupt igroup->last_sequence if the
wrapped value exceeds the previous sequence number.

[ ... ]

[Severity: Medium]
Is there a data race on struct kap_task_state between kap_bind_pid_locked()
and kap_ptrace_target()?

kap_bind_pid_locked() sets state->active = true before populating state->epoch
and state->igroup_name, while kap_ptrace_target() reads those fields without
holding kap_lock:

CPU0                              CPU1
kapfs_write()
  spin_lock(&kap_lock)
  kap_bind_pid_locked()
    state->active = true
                                  kap_ptrace_access_check()
                                    kap_ptrace_target()
                                      if (child_state->active)
                                        strcmp(..., child_state->igroup_name)
                                          <-- reads empty or torn igroup_name
    state->epoch = igroup->epoch
    strscpy(state->igroup_name, ...)

A concurrent ptrace check can therefore misclassify same-igroup vs
cross-igroup ptrace operations and log a corrupted target in kap.events.

[Severity: Medium]
Can a thread or child being created in copy_process() escape binding when
kap_bind_pid_locked() runs concurrently?

kap_task_alloc() copies the parent's kap_task_state early in copy_process(),
long before the new task is linked onto signal->thread_head under siglock:

CPU0                              CPU1
copy_process()
  security_task_alloc()
    kap_task_alloc()
      spin_lock(&kap_lock)
      /* old->active is false */
      spin_unlock(&kap_lock)
                                  kapfs_write()
                                    spin_lock(&kap_lock)
                                    kap_bind_pid_locked()
                                      for_each_thread(task, t)
                                        state->active = true
                                    spin_unlock(&kap_lock)
  write_lock_irq(&tasklist_lock)
  list_add_tail_rcu(&p->thread_node,
                    &p->signal->thread_head)
    <-- p runs with state->active == false

The new thread or child process is missed by both kap_task_alloc() and
for_each_thread(), leaving it running unbound even though writing the process
PID to kap.procs succeeded.

[ ... ]

[ ... ]

[ ... ]

[ ... ]

+	}

[ ... ]

[Severity: Medium]
Does kapfs_mkdir() leak a dentry reference on each entry in files[]?

d_alloc_name() returns each child dentry with a refcount of 1, and
kapfs_instantiate() -> d_make_persistent() takes an additional reference via
dget_dlock().  Unlike the directory dentry argument whose lookup reference is
dropped by the VFS caller, files[i] is local to kapfs_mkdir() and needs
dput(files[i]) after kapfs_instantiate().

[ ... ]

[Severity: Medium]
Should kapfs_fill_super() initialize sb->s_op, sb->s_blocksize, and
sb->s_blocksize_bits?

Without sb->s_op = &simple_super_operations, statfs() and fstatfs() on
/sys/kernel/security/kap/igroups and its entries fail with -ENOSYS, and
leaving sb->s_blocksize and sb->s_blocksize_bits at zero causes stat() to
report st_blksize = 1.

[ ... ]

[Severity: Medium]
Doesn't jumping onto the unattached kapfs_mnt cause d_path() in KAP's LSM
hooks to resolve paths under /sys/kernel/security/kap/igroups as
/<igroup>/<file>?

kapfs_mnt is created with kern_mount() and is not attached to any mountpoint,
so when kap_file_target(), kap_lsm_file_path_alloc(), or
kap_lsm_check_dir_entry() calls d_path(), walk-back stops at
kapfs_mnt->mnt_root and omits the /sys/kernel/security/kap/igroups prefix:

security/kap/kap_lsm.c:kap_lsm_file_path_alloc() {
    ...
	path = d_path(&file->f_path, buf, PATH_MAX);
	if (IS_ERR(path)) {
		kfree(buf);
		return NULL;
	}
    ...
}

File rules targeting /sys or /sys/kernel/security/kap will therefore not match
accesses under igroups/, and kap.events will log root-relative target paths.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110202246.274-1-harisudhan@armoriq.io?part=4