Re: [RFC PATCH v2 2/2] landlock: Add a test for REFER layer composition with network layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] landlock: use of handle instead of real full name in
  Signed-off-by
- [Low] selftests/landlock: redundant dir_s1d3 rule in
  refer_with_net_layer()
--

commit a26cce886a00516c15353a37b118df17895f1736
Author: pandaWall <mangosteen922@126.com>

> landlock: Add a test for REFER layer composition with network layer
>
> Check that stacking a network-only ruleset (i.e. a layer handling no
> filesystem access right) on top of a filesystem ruleset still allows
> different-parent renames and exchanges allowed by the filesystem layer.
> Before the fix, such a layer implicitly handled
> LANDLOCK_ACCESS_FS_REFER and denied these operations with EXDEV.
>
> Both parent directories get MAKE_REG because each is the destination
> parent of one rename direction (and of the exchange), as
> current_check_refer_path() requires the moved file type's access right
> on both parents.
>
> Signed-off-by: pandaWall <mangosteen922@126.com>

[Severity: High]
Per Documentation/process/submitting-patches.rst (Developer's Certificate
of Origin), the Author header and Signed-off-by trailer must use a real
full human name rather than a single-word handle ("pandaWall").

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index f725882fa4a78..cf11281639c67 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2570,6 +2570,49 @@ const struct rule layer_dir_s2d1_execute[] = {
>  	{},
>  };
>  
> +/*
> + * Tests that a layer not handling filesystem access rights does not
> + * implicitly handle LANDLOCK_ACCESS_FS_REFER, which would otherwise deny
> + * different-parent renames and links stacked on top of a filesystem ruleset.
> + */
> +TEST_F_FORK(layout1, refer_with_net_layer)
> +{
> +	const struct rule layer_fs[] = {
> +		{
> +			.path = dir_s1d2,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MAKE_REG,
> +		},
> +		{
> +			.path = dir_s1d3,
> +			.access = LANDLOCK_ACCESS_FS_MAKE_REG,
> +		},
> +		{},
> +	};
> +	const struct landlock_ruleset_attr ruleset_net = {
> +		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> +	};
> +
> +	/*
> +	 * layer_fs alone: different-parent renames are allowed.  Both
> +	 * parent directories need MAKE_REG because each is the destination
> +	 * of one rename direction, and the destination parent of an
> +	 * exchange.
> +	 */
> +	enforce_fs(_metadata, LANDLOCK_ACCESS_FS_REFER |
> +				      LANDLOCK_ACCESS_FS_MAKE_REG,
> +		   layer_fs);

[Severity: Low]
Is the second rule in layer_fs[] on dir_s1d3 redundant?

In layout1, dir_s1d3 (TMP_DIR/s1d1/s1d2/s1d3) is a subdirectory of
dir_s1d2, so the first rule on dir_s1d2 already grants both
LANDLOCK_ACCESS_FS_REFER and LANDLOCK_ACCESS_FS_MAKE_REG to dir_s1d3.
Moreover, current_check_refer_path() requires LANDLOCK_ACCESS_FS_REFER on
both parent directories, so if dir_s1d3 did not inherit from dir_s1d2, the
second rule omitting LANDLOCK_ACCESS_FS_REFER would fail with EXDEV.

Because layer_fs[0] grants all handled filesystem rights in the domain on
dir_s1d2, collect_domain_accesses() in current_check_refer_path() also
returns true for both allow_parent1 and allow_parent2 and returns 0 early
before is_access_to_paths_allowed() checks LANDLOCK_ACCESS_FS_MAKE_REG.

Also, placing refer_with_net_layer here separates the
layer_dir_s1d1_refer, layer_dir_s1d1_execute, and layer_dir_s2d1_execute
rule definitions right above from the refer_denied_by_default1..4 tests
below that use them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112052713.859515-1-mangosteen922@126.com?part=2