Re: [PATCH v4 8/30] landlock: Add AUDIT_LANDLOCK_DENY and log ptrace denials

[Paul Moore <paul@paul-moore.com>]

On Jan  8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> Add a new AUDIT_LANDLOCK_DENY record type dedicated to any Landlock
> denials.

...

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 75e21a135483..60c909c396c0 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -33,7 +33,7 @@
>   * 1100 - 1199 user space trusted application messages
>   * 1200 - 1299 messages internal to the audit daemon
>   * 1300 - 1399 audit event messages
> - * 1400 - 1499 SE Linux use
> + * 1400 - 1499 access control messages
>   * 1500 - 1599 kernel LSPP events
>   * 1600 - 1699 kernel crypto events
>   * 1700 - 1799 kernel anomaly records
> @@ -146,6 +146,7 @@
>  #define AUDIT_IPE_ACCESS	1420	/* IPE denial or grant */
>  #define AUDIT_IPE_CONFIG_CHANGE	1421	/* IPE config change */
>  #define AUDIT_IPE_POLICY_LOAD	1422	/* IPE policy load */
> +#define AUDIT_LANDLOCK_DENY	1423	/* Landlock denial */

I didn't have an opportunity to respond to your reply to my v3 comments
before you posted v4, but I see you've decided to stick with _DENY as
opposed to _ACCESS (or something similar).  Let me copy your reply
below so I can respond appropriately ...

> A stronger type with the "denied" semantic makes more sense to me,
> especially for Landlock which is unprivileged, and it makes it clear
> that it should only impact performance and log size (i.e. audit log
> creation) for denied actions.

This is not consistent with how audit is typically used.  Please
convert to AUDIT_LANDLOCK_ACCESS, or something similar.

> The next patch
> series will also contain a new kind of audit rule to specifically
> identify the origin of the policy that created this denied event, which
> should make more sense.

Generally speaking audit only wants to support a small number of message
types dedicated to a specific LSM.  If you're aware of additional message
types that you plan to propose in a future patchset, it's probably a
time to discuss those now.

> Because of its unprivileged nature, Landlock will never log granted
> accesses by default.  In the future, we might want a permissive-like
> mode for Landlock, but this will be optional, and I would also strongly
> prefer to add new audit record types for new semantics.

Once again, this isn't consistent with how audit is typically used and
I'm not seeing a compelling reason to rework how things are done.  Please
stick with encoding the success/failure, accept/reject, etc. states in
audit record fields, not the message types themselves.

--
paul-moore.com