[PATCH v2 6/6] Implement quiet for optional accesses

[Mickaël Salaün]

On Sun, Oct 19, 2025 at 06:45:55PM +0100, Tingmao Wang wrote:
> On 10/15/25 20:09, Mickaël Salaün wrote:
> > [...]
> > On Sun, Oct 05, 2025 at 06:55:29PM +0100, Tingmao Wang wrote:
> >> [..]
> >> diff --git a/security/landlock/domain.c b/security/landlock/domain.c
> >> index a647b68e8d06..0f611ad516be 100644
> >> --- a/security/landlock/domain.c
> >> +++ b/security/landlock/domain.c
> >> @@ -212,6 +212,29 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,
> >>  	return deny_masks;
> >>  }
> >>  
> > 
> > Just using u8 is confusing.  Please document what is the "type" of the
> > returned value, and use a dedicated typedef instead of u8 (see my other
> > comment about static_assert).  This typedef should probably be named
> > optional_access_t and have a size less or equal to access_t's one.
> > 
> >> +u8 landlock_get_quiet_optional_accesses(
> >> +	const access_mask_t all_existing_optional_access,
> >> +	const deny_masks_t deny_masks,
> >> +	const struct collected_rule_flags rule_flags)
> >> +{
> >> +	const unsigned long access_opt = all_existing_optional_access;
> >> +	size_t access_index = 0;
> >> +	unsigned long access_bit;
> >> +	u8 quiet_optional_accesses = 0;
> > 
> > As for deny_masks_t, we should define an "optional_access_t" type with
> > appropriate safeguard to guarantee that it can always hold all optional
> > access rights (see static_assert for deny_masks_t in access.h).
> > 
> > We should also copy the WARN_ON_ONCE() check from
> > get_layer_from_deny_masks().
> 
> I don't see how that WARN_ON_ONCE is applicable here since we're no longer
> dealing with the `optional_access`...  Can you clarify?

WARN_ON_ONCE(access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL);

> 
> > 
> >> [...]
> >> @@ -80,13 +88,24 @@ struct landlock_file_security {
> >>  	struct landlock_cred_security fown_subject;
> >>  };
> >>  
> >> -#ifdef CONFIG_AUDIT
> >> +static void build_check_file_security(void)
> > 
> > You can move this function to fs.c and call it in
> > hook_file_alloc_security() instead.
> > 
> >> +{
> >> +	const struct landlock_file_security file_sec = {
> >> +		.quiet_optional_accesses = ~0,
> >> +		.fown_layer = ~0,
> >> +	};
> >> +
> >> +	/*
> >> +	 * Make sure quiet_optional_accesses has enough bits to cover all
> >> +	 * optional accesses
> >> +	 */
> >> +	BUILD_BUG_ON(__const_hweight8(file_sec.quiet_optional_accesses) <
> > 
> > We should be able to use HWEIGHT() instead.
> 
> I tried it and unfortunately it doesn't seem to work :(
> 
> 	security/landlock/fs.c: In function ‘build_check_file_security’:
> 	./include/linux/compiler.h:201:82: error: expression in static assertion is not constant
> 	  201 | #define __BUILD_BUG_ON_ZERO_MSG(e, msg, ...) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
> 	      |                                                                                  ^~~~
> 	././include/linux/compiler_types.h:577:23: note: in definition of macro ‘__compiletime_assert’
> 	  577 |                 if (!(condition))                                       \
> 	      |                       ^~~~~~~~~
> 	././include/linux/compiler_types.h:597:9: note: in expansion of macro ‘_compiletime_assert’
> 	  597 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> 	      |         ^~~~~~~~~~~~~~~~~~~
> 	./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> 	   39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> 	      |                                     ^~~~~~~~~~~~~~~~~~
> 	./include/linux/build_bug.h:50:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> 	   50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> 	      |         ^~~~~~~~~~~~~~~~
> 	security/landlock/fs.c:1769:9: note: in expansion of macro ‘BUILD_BUG_ON’
> 	 1769 |         BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
> 	      |         ^~~~~~~~~~~~
> 	./include/linux/build_bug.h:17:9: note: in expansion of macro ‘__BUILD_BUG_ON_ZERO_MSG’
> 	   17 |         __BUILD_BUG_ON_ZERO_MSG(e, ##__VA_ARGS__, #e " is true")
> 	      |         ^~~~~~~~~~~~~~~~~~~~~~~
> 	./include/asm-generic/bitops/const_hweight.h:37:23: note: in expansion of macro ‘BUILD_BUG_ON_ZERO’
> 	   37 | #define HWEIGHT64(w) (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_hweight64(w))
> 	      |                       ^~~~~~~~~~~~~~~~~
> 	./include/asm-generic/bitops/const_hweight.h:42:22: note: in expansion of macro ‘HWEIGHT64’
> 	   42 | #define HWEIGHT(w)   HWEIGHT64((u64)w)
> 	      |                      ^~~~~~~~~
> 	security/landlock/fs.c:1769:22: note: in expansion of macro ‘HWEIGHT’
> 	 1769 |         BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
> 	      |                      ^~~~~~~
> 

Indeed, it works with Clang but not GCC.

Consistently always using __const_hweight64() might help avoid future
issue if quiet_optional_accesses grows more than 8 bits though.

> > 
> >> +		     __const_hweight64(_LANDLOCK_ACCESS_FS_OPTIONAL));
> >> +	/* Makes sure all layers can be identified. */
> >> +	BUILD_BUG_ON(file_sec.fown_layer < LANDLOCK_MAX_NUM_LAYERS - 1);
> >> +}
> >>  
> >> -/* Makes sure all layers can be identified. */
> >> -/* clang-format off */
> >> -static_assert((typeof_member(struct landlock_file_security, fown_layer))~0 >=
> >> -	      LANDLOCK_MAX_NUM_LAYERS);
> >> -/* clang-format off */
> >> +#ifdef CONFIG_AUDIT
> >>  
> >>  #endif /* CONFIG_AUDIT */
> >>  
> >> @@ -107,6 +126,7 @@ struct landlock_superblock_security {
> >>  static inline struct landlock_file_security *
> >>  landlock_file(const struct file *const file)
> >>  {
> >> +	build_check_file_security();
> >>  	return file->f_security + landlock_blob_sizes.lbs_file;
> >>  }
> >>  
> >> -- 
> >> 2.51.0
>