[PATCH] landlock: Work around randstruct unnamed static initializer support

[Günther Noack]

Hello Kees!

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:
> 
> 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.

I am still struggling to understand the boundaries of what the randstruct plugin
supports and what it fails on.  Could you please clarify?  (Specific questions
below.)


> Reported-by: "Dr. David Alan Gilbert" <linux at treblig.org>
> Closes: https://lore.kernel.org/lkml/Z_PRaKx7q70MKgCA@gallifrey/
> Reported-by: Mark Brown <broonie at kernel.org>
> Closes: https://lore.kernel.org/lkml/20250407-kbuild-disable-gcc-plugins-v1-1-5d46ae583f5e@kernel.org/
> Reported-by: WangYuli <wangyuli at uniontech.com>
> Closes: https://lore.kernel.org/lkml/337D5D4887277B27+3c677db3-a8b9-47f0-93a4-7809355f1381@uniontech.com/
> Signed-off-by: Kees Cook <kees at kernel.org>
> ---
> Cc: "Mickaël Salaün" <mic at digikod.net>
> Cc: "Günther Noack" <gnoack at google.com>
> Cc: Mark Brown <broonie at kernel.org>
> Cc: Arnd Bergmann <arnd at arndb.de>
> Cc: Paul Moore <paul at paul-moore.com>
> Cc: James Morris <jmorris at namei.org>
> Cc: "Serge E. Hallyn" <serge at hallyn.com>
> Cc: <linux-security-module at vger.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);
>  }

If I understood the commit message correctly, we are giving a name ("request")
to the struct landlock_request here, because the randstruct plugin needs that if
the constructed struct (recursively?) contains a randstruct-enabled member, like
.audit.u.path in this case?

I am surprised that you pulled the assignment to "request.audit.u.path" out of
the struct initialization expression though.  Was that also necessary for
randstruct to work?

Would it have worked to initialize it inline instead, like this?

	struct landlock_request request = {
		.type           = LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY,
		.audit.type     = LSM_AUDIT_DATA_PATH,
    .audit.u.path   = *path,
		.layer_plus_one = handle_layer + 1,
	};


>  
>  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;

Same question here, I guess. This could not have been written like this?

  struct lsm_ioctlop_audit audit_log = {
    .path = file->f_path,
    .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
> 

That being said, the code transformation in this patch is obviously correct, and
it's good to fix the build, so looks good:

Reviewed-by: Günther Noack <gnoack at google.com>

But as Mickaël also said, it seems worrying that otherwise correct C code does
not compile with that plugin, especially when the conditions under which that
happens are not clear.

I would also like to understand this better, so that we can avoid tripping over
it in the future.

Thanks!
-Günther