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

[Tingmao Wang]

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?

> 
>> [...]
>> @@ -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) <
	      |                      ^~~~~~~

> 
>> +		     __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