linux-hardening - Re: [PATCH] landlock: Work around randstruct unnamed static initializer support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250422.eetheiPu6aiH@digikod.net>
Date: Tue, 22 Apr 2025 14:25:13 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Kees Cook <kees@...nel.org>
Cc: "Dr. David Alan Gilbert" <linux@...blig.org>, 
	Mark Brown <broonie@...nel.org>, WangYuli <wangyuli@...ontech.com>, 
	Günther Noack <gnoack@...gle.com>, Arnd Bergmann <arnd@...db.de>, Paul Moore <paul@...l-moore.com>, 
	James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] landlock: Work around randstruct unnamed static
 initializer support

On Sun, Apr 20, 2025 at 05:08:59PM -0700, Kees Cook wrote:
> Unnamed static initializers aren't supported by the randstruct GCC
> plugin. Quoting the plugin, "set up a bogus anonymous struct field
> designed to error out on unnamed struct initializers as gcc provides
> no other way to detect such code". That is exactly what happens
> with the landlock code, so adjust the static initializers for structs
> lsm_ioctlop_audit and landlock_request that contain a randomized structure
> (struct path) to use named variables, which avoids the intentional
> GCC crashes:

This is not a sustainable solution.  Could we fix the plugin instead?
This new Landlock change may be the first to trigger this plugin bug but
it will probably not be the last to use unnamed static initializers.
Forbidding specific C constructs should be documented.

As a temporary solution (not sure if it's worth it, nor a good idea),
could handling of types used in unnamed static initializers be ignored
by this plugin?  I guess [1] is already a temporary solution though.
[1] https://lore.kernel.org/all/20250409151154.work.872-kees@kernel.org/

> 
> security/landlock/fs.c: In function 'hook_file_ioctl_common':
> security/landlock/fs.c:1745:61: internal compiler error: in count_type_elements, at expr.cc:7092
>  1745 |                         .u.op = &(struct lsm_ioctlop_audit) {
>       |                                                             ^
> 
> security/landlock/fs.c: In function 'log_fs_change_topology_path':
> security/landlock/fs.c:1379:65: internal compiler error: in count_type_elements, at expr.cc:7092
>  1379 |         landlock_log_denial(subject, &(struct landlock_request) {
>       |                                                                 ^
> 
> We went 8 years before tripping over this! With this patch landed,
> we can enable COMPILE_TEST builds with the randstruct GCC plugin again.
> 
> Reported-by: "Dr. David Alan Gilbert" <linux@...blig.org>
> Closes: https://lore.kernel.org/lkml/Z_PRaKx7q70MKgCA@gallifrey/
> Reported-by: Mark Brown <broonie@...nel.org>
> Closes: https://lore.kernel.org/lkml/20250407-kbuild-disable-gcc-plugins-v1-1-5d46ae583f5e@kernel.org/
> Reported-by: WangYuli <wangyuli@...ontech.com>
> Closes: https://lore.kernel.org/lkml/337D5D4887277B27+3c677db3-a8b9-47f0-93a4-7809355f1381@uniontech.com/
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---
> Cc: "Mickaël Salaün" <mic@...ikod.net>
> Cc: "Günther Noack" <gnoack@...gle.com>
> Cc: Mark Brown <broonie@...nel.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Paul Moore <paul@...l-moore.com>
> Cc: James Morris <jmorris@...ei.org>
> Cc: "Serge E. Hallyn" <serge@...lyn.com>
> Cc: <linux-security-module@...r.kernel.org>
> ---
>  security/landlock/fs.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6fee7c20f64d..b2818afb0503 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -1376,14 +1376,14 @@ static void
>  log_fs_change_topology_path(const struct landlock_cred_security *const subject,
>  			    size_t handle_layer, const struct path *const path)
>  {
> -	landlock_log_denial(subject, &(struct landlock_request) {
> +	struct landlock_request request = {
>  		.type = LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY,
> -		.audit = {
> -			.type = LSM_AUDIT_DATA_PATH,
> -			.u.path = *path,
> -		},
> +		.audit.type = LSM_AUDIT_DATA_PATH,
>  		.layer_plus_one = handle_layer + 1,
> -	});
> +	};
> +	request.audit.u.path = *path;
> +
> +	landlock_log_denial(subject, &request);
>  }
>  
>  static void log_fs_change_topology_dentry(
> @@ -1720,6 +1720,7 @@ static int hook_file_truncate(struct file *const file)
>  static int hook_file_ioctl_common(const struct file *const file,
>  				  const unsigned int cmd, const bool is_compat)
>  {
> +	struct lsm_ioctlop_audit audit_log;
>  	access_mask_t allowed_access = landlock_file(file)->allowed_access;
>  
>  	/*
> @@ -1738,14 +1739,13 @@ static int hook_file_ioctl_common(const struct file *const file,
>  				  is_masked_device_ioctl(cmd))
>  		return 0;
>  
> +	audit_log.path = file->f_path;
> +	audit_log.cmd = cmd;
>  	landlock_log_denial(landlock_cred(file->f_cred), &(struct landlock_request) {
>  		.type = LANDLOCK_REQUEST_FS_ACCESS,
>  		.audit = {
>  			.type = LSM_AUDIT_DATA_IOCTL_OP,
> -			.u.op = &(struct lsm_ioctlop_audit) {
> -				.path = file->f_path,
> -				.cmd = cmd,
> -			},
> +			.u.op = &audit_log,
>  		},
>  		.all_existing_optional_access = _LANDLOCK_ACCESS_FS_OPTIONAL,
>  		.access = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> -- 
> 2.34.1
> 
>