Re: [RFC PATCH v2 27/41] audit: log UUID and GUID ids in full

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] audit: xfrm_audit_helper_usrinfo() still logs UUID/GUID auid as 4294967295
- [Medium] audit: Landlock log_domain() still logs UUID/GUID creator uid as 4294967295
--

commit 91e2cc101c7d60a9061395bf85632c87b9500902
Author: Jan Koester <jan.koester@tuxist.de>

audit: log UUID and GUID ids in full

This commit replaces from_kuid() and from_kgid() calls in audit record
formatting with KUID_STR() and KGID_STR() so that uuid and guid user and
group identifiers are logged in their text form rather than as -1
(4294967295).

>     audit: log UUID and GUID ids in full
>     
>     Audit records print ids with from_kuid(&init_user_ns, ...), which gives
>     -1 for a UUID or GUID. 4294967295 is what audit uses for "unset", so a
>     UUID user's actions would be logged as if there were no login user.
>     
>     Print all ids in audit records with KUID_STR()/KGID_STR(): classic ids
>     come out exactly as before (signed where the format was "%d"), UUIDs and
>     GUIDs in their text form. The binary AUDIT_SIGNAL_INFO reply keeps its
>     32 bit field.
>     
>     Assisted-by: Claude:claude-opus-5-5
>     
>     Signed-off-by: Jan Koester <jan.koester@tuxist.de>

[Severity: Medium]
Should xfrm_audit_helper_usrinfo() in include/net/xfrm.h also be converted to
use KUID_STR()?

When a task running under a uuid or guid loginuid adds or deletes an ipsec
policy or state, xfrm_audit_policy_add(), xfrm_audit_policy_delete(),
xfrm_audit_state_add(), or xfrm_audit_state_delete() calls
xfrm_audit_helper_usrinfo() with task_valid set to true:

include/net/xfrm.h:xfrm_audit_helper_usrinfo() {
	const unsigned int auid = from_kuid(&init_user_ns, task_valid ?
					    audit_get_loginuid(current) :
					    INVALID_UID);
	const unsigned int ses = task_valid ? audit_get_sessionid(current) :
		AUDIT_SID_UNSET;

	audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
	audit_log_task_context(audit_buf);
}

Because from_kuid(&init_user_ns, ...) returns (uid_t)-1 for a uuid or guid,
AUDIT_MAC_IPSEC_EVENT records will still log auid=4294967295 (unset) instead
of formatting task_valid ? audit_get_loginuid(current) : INVALID_UID with
KUID_STR() and %s.

[Severity: Medium]
What about Landlock domain audit logging in security/landlock/domain.c,
security/landlock/domain.h, and security/landlock/audit.c?

When a task with a uuid or guid current_uid() restricts itself via
landlock_restrict_self() -> landlock_init_hierarchy_log() ->
get_current_details(), the creator's uid is converted with from_kuid() and
stored as a 32-bit uid_t in struct landlock_details:

security/landlock/domain.h:
struct landlock_details {
	struct pid *pid;
	uid_t uid;
	char comm[TASK_COMM_LEN];
	char exe_path[];
};

security/landlock/domain.c:get_current_details() {
	...
	memcpy(details->exe_path, path_str, path_size);
	details->pid = get_pid(task_tgid(current));
	details->uid = from_kuid(&init_user_ns, current_uid());
	get_task_comm(details->comm, current);
	return details;
}

When log_domain() in security/landlock/audit.c later emits the
AUDIT_LANDLOCK_DOMAIN record, it logs hierarchy->details->uid with %u:

security/landlock/audit.c:log_domain() {
	...
	audit_log_format(
		ab,
		"domain=%llx status=allocated mode=enforcing pid=%d uid=%u exe=",
		hierarchy->id, pid_nr(hierarchy->details->pid),
		hierarchy->details->uid);
	...
}

For a uuid or guid user, from_kuid(&init_user_ns, current_uid()) yields
(uid_t)-1, causing log_domain() to log uid=4294967295. Could struct
landlock_details store a kuid_t instead and format it with KUID_STR() and
uid=%s in log_domain()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=27