Re: [PATCH v2 3/6] landlock/audit: Check for quiet flag in landlock_log_denial

["Mickaël Salaün" <mic@digikod.net>]

Just use "landlock: " as subject prefix.

On Sun, Oct 05, 2025 at 06:55:26PM +0100, Tingmao Wang wrote:
> Suppresses logging if the flag is effective on the youngest layer.
> 
> This does not handle optional access logging yet - to do that correctly we
> will need to expand deny_masks to support representing "don't log
> anything" in a later commit.
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
> 
> Changes since v1:
> - Supports the new quiet access masks.
> - Support quieting scope requests (but not ptrace and attempted mounting
>   for now)
> 
>  security/landlock/audit.c   | 70 +++++++++++++++++++++++++++++++++++--
>  security/landlock/audit.h   |  3 +-
>  security/landlock/fs.c      | 18 +++++-----
>  security/landlock/net.c     |  3 +-
>  security/landlock/ruleset.h |  5 +++
>  security/landlock/task.c    | 12 +++----
>  6 files changed, 92 insertions(+), 19 deletions(-)
> 
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index c52d079cdb77..ec00b7dd00c5 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -381,19 +381,39 @@ static bool is_valid_request(const struct landlock_request *const request)
>  	return true;
>  }
>  
> +static access_mask_t
> +pick_access_mask_for_req_type(const enum landlock_request_type type,

pick_access_mask_for_request_type

> +			      const struct access_masks access_masks)
> +{
> +	switch (type) {
> +	case LANDLOCK_REQUEST_FS_ACCESS:
> +		return access_masks.fs;
> +	case LANDLOCK_REQUEST_NET_ACCESS:
> +		return access_masks.net;
> +	default:
> +		WARN_ONCE(1, "Invalid request type %d passed to %s", type,
> +			  __func__);
> +		return 0;
> +	}
> +}
> +
>  /**
>   * landlock_log_denial - Create audit records related to a denial
>   *
>   * @subject: The Landlock subject's credential denying an action.
>   * @request: Detail of the user space request.
> + * @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if
> + * this is a scope request (no particular object involved).
>   */
>  void landlock_log_denial(const struct landlock_cred_security *const subject,
> -			 const struct landlock_request *const request)
> +			 const struct landlock_request *const request,
> +			 const struct collected_rule_flags rule_flags)
>  {
>  	struct audit_buffer *ab;
>  	struct landlock_hierarchy *youngest_denied;
>  	size_t youngest_layer;
> -	access_mask_t missing;
> +	access_mask_t missing, quiet_mask;
> +	bool quiet_flag_on_rule = false, quiet_applicable_to_access = false;
>  
>  	if (WARN_ON_ONCE(!subject || !subject->domain ||
>  			 !subject->domain->hierarchy || !request))
> @@ -436,6 +456,52 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
>  	if (!audit_enabled)
>  		return;
>  
> +	/*
> +	 * Checks if the object is marked quiet by the layer that denied the
> +	 * request.  If it's a different layer that marked it as quiet, but
> +	 * that layer is not the one that denied the request, we should still
> +	 * audit log the denial.
> +	 */
> +	quiet_flag_on_rule = !!(rule_flags.quiet_masks & BIT(youngest_layer));
> +
> +	if (quiet_flag_on_rule) {
> +		/*
> +		 * This is not a scope request, since rule_flags is not zero.  We
> +		 * now check if the denied requests are all covered by the layer's
> +		 * quiet access bits.
> +		 */
> +		quiet_mask = pick_access_mask_for_req_type(
> +			request->type, youngest_denied->quiet_masks);
> +		quiet_applicable_to_access = (quiet_mask & missing) == missing;
> +
> +		if (quiet_applicable_to_access)
> +			return;
> +	} else {
> +		quiet_mask = youngest_denied->quiet_masks.scope;
> +		switch (request->type) {
> +		case LANDLOCK_REQUEST_SCOPE_SIGNAL:
> +			quiet_applicable_to_access =
> +				!!(quiet_mask & LANDLOCK_SCOPE_SIGNAL);
> +			break;
> +		case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET:
> +			quiet_applicable_to_access =
> +				!!(quiet_mask &
> +				   LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
> +			break;
> +		/*
> +		 * Leave LANDLOCK_REQUEST_PTRACE and
> +		 * LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are
> +		 * never quiet
> +		 */

This also covers the case where the object is not quiet.

> +		default:
> +			break;
> +		}

I find this if/else block a bit verbose but I didn't find a better
way...

> +
> +		if (quiet_applicable_to_access) {
> +			return;
> +		}

We can still move this quiet_applicable_to_access check after the block
(and without the curly braces).

> +	}
> +
>  	/* Checks if the current exec was restricting itself. */
>  	if (subject->domain_exec & BIT(youngest_layer)) {
>  		/* Ignores denials for the same execution. */

This domain_exec block would be better before the quiet_flag_on_rule
use.

> diff --git a/security/landlock/audit.h b/security/landlock/audit.h
> index 92428b7fc4d8..80cf085465e3 100644
> --- a/security/landlock/audit.h
> +++ b/security/landlock/audit.h
> @@ -56,7 +56,8 @@ struct landlock_request {
>  void landlock_log_drop_domain(const struct landlock_hierarchy *const hierarchy);
>  
>  void landlock_log_denial(const struct landlock_cred_security *const subject,
> -			 const struct landlock_request *const request);
> +			 const struct landlock_request *const request,
> +			 const struct collected_rule_flags rule_flags);
>  
>  #else /* CONFIG_AUDIT */
>  
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index b566ae498df5..1ccef1c2959f 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -984,7 +984,7 @@ static int current_check_access_path(const struct path *const path,
>  				       NULL, 0, NULL, NULL, NULL, NULL))
>  		return 0;
>  
> -	landlock_log_denial(subject, &request);
> +	landlock_log_denial(subject, &request, rule_flags);
>  	return -EACCES;
>  }
>  
> @@ -1194,7 +1194,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  			    &request1, NULL, 0, NULL, NULL, NULL, NULL))
>  			return 0;
>  
> -		landlock_log_denial(subject, &request1);
> +		landlock_log_denial(subject, &request1, rule_flags_parent1);
>  		return -EACCES;
>  	}
>  
> @@ -1243,11 +1243,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  
>  	if (request1.access) {
>  		request1.audit.u.path.dentry = old_parent;
> -		landlock_log_denial(subject, &request1);
> +		landlock_log_denial(subject, &request1, rule_flags_parent1);
>  	}
>  	if (request2.access) {
>  		request2.audit.u.path.dentry = new_dir->dentry;
> -		landlock_log_denial(subject, &request2);
> +		landlock_log_denial(subject, &request2, rule_flags_parent2);
>  	}
>  
>  	/*
> @@ -1403,7 +1403,7 @@ log_fs_change_topology_path(const struct landlock_cred_security *const subject,
>  			.u.path = *path,
>  		},
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	}, no_rule_flags);
>  }
>  
>  static void log_fs_change_topology_dentry(
> @@ -1417,7 +1417,7 @@ static void log_fs_change_topology_dentry(
>  			.u.dentry = dentry,
>  		},
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	}, no_rule_flags);
>  }
>  
>  /*
> @@ -1705,7 +1705,7 @@ static int hook_file_open(struct file *const file)
>  
>  	/* Sets access to reflect the actual request. */
>  	request.access = open_access_request;
> -	landlock_log_denial(subject, &request);
> +	landlock_log_denial(subject, &request, rule_flags);
>  	return -EACCES;
>  }
>  
> @@ -1735,7 +1735,7 @@ static int hook_file_truncate(struct file *const file)
>  #ifdef CONFIG_AUDIT
>  		.deny_masks = landlock_file(file)->deny_masks,
>  #endif /* CONFIG_AUDIT */
> -	});
> +	}, no_rule_flags);
>  	return -EACCES;
>  }
>  
> @@ -1774,7 +1774,7 @@ static int hook_file_ioctl_common(const struct file *const file,
>  #ifdef CONFIG_AUDIT
>  		.deny_masks = landlock_file(file)->deny_masks,
>  #endif /* CONFIG_AUDIT */
> -	});
> +	}, no_rule_flags);
>  	return -EACCES;
>  }
>  
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index bddbe93d69fd..0587aa3d6d0f 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -193,7 +193,8 @@ static int current_check_access_socket(struct socket *const sock,
>  				    .access = access_request,
>  				    .layer_masks = &layer_masks,
>  				    .layer_masks_size = ARRAY_SIZE(layer_masks),
> -			    });
> +			    },
> +			    rule_flags);
>  	return -EACCES;
>  }
>  
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 43d59c7116e5..6f44804c2c9b 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -58,6 +58,11 @@ struct collected_rule_flags {
>  	layer_mask_t quiet_masks;
>  };
>  
> +/**
> + * no_rule_flags - Convenience constant for an empty collected_rule_flags
> + */
> +static const struct collected_rule_flags no_rule_flags = { 0 };

You can remove the "0" for consistency.

> +
>  /**
>   * union landlock_key - Key of a ruleset's red-black tree
>   */
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 2385017418ca..d5bd9a1b8467 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -115,7 +115,7 @@ static int hook_ptrace_access_check(struct task_struct *const child,
>  				.u.tsk = child,
>  			},
>  			.layer_plus_one = parent_subject->domain->num_layers,
> -		});
> +		}, no_rule_flags);
>  
>  	return err;
>  }
> @@ -161,7 +161,7 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  			.u.tsk = current,
>  		},
>  		.layer_plus_one = parent_subject->domain->num_layers,
> -	});
> +	}, no_rule_flags);
>  	return err;
>  }
>  
> @@ -290,7 +290,7 @@ static int hook_unix_stream_connect(struct sock *const sock,
>  			},
>  		},
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	}, no_rule_flags);
>  	return -EPERM;
>  }
>  
> @@ -327,7 +327,7 @@ static int hook_unix_may_send(struct socket *const sock,
>  			},
>  		},
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	}, no_rule_flags);
>  	return -EPERM;
>  }
>  
> @@ -383,7 +383,7 @@ static int hook_task_kill(struct task_struct *const p,
>  			.u.tsk = p,
>  		},
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	}, no_rule_flags);
>  	return -EPERM;
>  }
>  
> @@ -426,7 +426,7 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,
>  #ifdef CONFIG_AUDIT
>  		.layer_plus_one = landlock_file(fown->file)->fown_layer + 1,
>  #endif /* CONFIG_AUDIT */
> -	});
> +	}, no_rule_flags);
>  	return -EPERM;
>  }
>  
> -- 
> 2.51.0
>