Re: [PATCH v3 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 3 potential
issues to consider:
- [High] selftests/landlock: non-compliant handle pandaWall in
  Signed-off-by
- [Medium] selftests/landlock: ungranted MAKE_REG in
  refer_with_net_layer() fails
- [Low] selftests/landlock: stray blank line removal before
  refer_denied_by_default1
--

commit d4e80b85fa92ac6a7a9cb938ea00f1dbb1f7a137
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.
>
> A single rule on dir_s1d2 is enough: it covers its subdirectory
> dir_s1d3, and grants all the filesystem access rights handled by the
> domain on both parents of the tested renames.
>
> Signed-off-by: pandaWall <mangosteen922@126.com>

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

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index f725882fa4a78..94b339252f5db 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2569,7 +2569,6 @@ const struct rule layer_dir_s2d1_execute[] = {
>  	},
>  	{},
>  };
> -
>  /*
>   * Tests precedence over renames: denied by default for different parent
>   * directories, *with* a rule matching a parent directory, but not directly

[Severity: Low]
This isn't a bug, but this hunk removes an unrelated blank line after
layer_dir_s2d1_execute[], and the commit subject prefix should be
"selftests/landlock:" to match other commits touching this file.

> @@ -2612,6 +2611,42 @@ TEST_F_FORK(layout1, refer_denied_by_default4)
>  				layer_dir_s1d1_refer);
>  }
>  
> +/*
> + * 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,
> +		},
> +		{},
> +	};
> +	const struct landlock_ruleset_attr ruleset_net = {
> +		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> +	};
> +
> +	/*
> +	 * layer_fs alone: different-parent renames are allowed.  A single
> +	 * rule on dir_s1d2 covers its subdirectory dir_s1d3 as well.
> +	 */
> +	enforce_fs(_metadata, LANDLOCK_ACCESS_FS_REFER |
> +				      LANDLOCK_ACCESS_FS_MAKE_REG,
> +		   layer_fs);
> +	ASSERT_EQ(0, test_rename(file1_s1d2, file1_s1d3));

[Severity: Medium]
Does this test fail on the first test_rename() with EACCES?

enforce_fs() makes the domain handle both LANDLOCK_ACCESS_FS_REFER and
LANDLOCK_ACCESS_FS_MAKE_REG, while layer_fs only grants
LANDLOCK_ACCESS_FS_REFER on dir_s1d2.  When reparenting a regular file
across directories, current_check_refer_path() also requires
LANDLOCK_ACCESS_FS_MAKE_REG on the destination parent if the domain handles
it, so test_rename() and test_exchange() return EACCES before the test can
reach the network-layer check.  Either layer_fs[0].access needs to include
LANDLOCK_ACCESS_FS_MAKE_REG, or enforce_fs() should only handle
LANDLOCK_ACCESS_FS_REFER.

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