Re: [PATCH] landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] landlock: single-word handle in Signed-off-by and text after
  trailers
- [Medium] selftests/landlock: missing MAKE_REG on dir_s1d2 breaks
  refer_with_net_layer
--

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

> landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER
>
> landlock_get_fs_access_mask() unconditionally ORs
> _LANDLOCK_ACCESS_FS_INITIALLY_DENIED (i.e. LANDLOCK_ACCESS_FS_REFER)
> into every layer's handled filesystem mask.  Layers created from
> rulesets that only handle other rule types (network ports, scopes) or
> only permissions therefore report REFER as handled even though
> landlock_upgrade_handled_layer_config() only adds the bit when
> handled.fs is non-zero, and no filesystem rule can ever clear a bit
> of such a layer.
>
> The consequence is that stacking any non-filesystem ruleset on top of
> a filesystem ruleset makes link(2), rename(2) and
> renameat2(RENAME_EXCHANGE) fail with -EXDEV/-EACCES even when each
> ruleset individually allows the operation: landlock_unmask_layers()
> requires every layer's mask to be cleared, and a non-filesystem
> layer's mask can never be.
>
> Only implicitly handle REFER for layers that already handle
> filesystem access rights.
>
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> Signed-off-by: pandaWall <mangosteen922@126.com>
>
> Add a corresponding regression test to the layout1 fs_test suite:
> stacking a network-only ruleset (handled_access_net only) on top of a
> filesystem ruleset allowing REFER and MAKE_REG must not deny
> different-parent rename/exchange.

[Severity: High]
Per Documentation/process/submitting-patches.rst, the Signed-off-by:
trailer and Author header must use a real full name rather than a
single-word handle or nickname ("pandaWall").  In addition, the final
paragraph describing the regression test should be placed before the
Fixes: and Signed-off-by: trailers.

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index f725882fa4a78..5588f0ee58625 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -2570,6 +2570,43 @@ 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,
> +		},
> +		{
> +			.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 rename is allowed. */
> +	enforce_fs(_metadata, LANDLOCK_ACCESS_FS_REFER |
> +				      LANDLOCK_ACCESS_FS_MAKE_REG,
> +		   layer_fs);
> +	ASSERT_EQ(0, test_rename(file1_s1d2, file1_s1d3));
> +	ASSERT_EQ(0, test_rename(file1_s1d3, file1_s1d2));
> +	ASSERT_EQ(0, test_exchange(file2_s1d2, file2_s1d3));
> +
> +	/* Stacks a network-only layer: still allowed. */
> +	drop_access_rights(_metadata, &ruleset_net);
> +	ASSERT_EQ(0, test_rename(file1_s1d2, file1_s1d3));
> +	ASSERT_EQ(0, test_rename(file1_s1d3, file1_s1d2));
> +	ASSERT_EQ(0, test_exchange(file2_s1d2, file2_s1d3));
> +}

[Severity: Medium]
Does layer_fs need LANDLOCK_ACCESS_FS_MAKE_REG on dir_s1d2 as well?

In layout1, dir_s1d2 is the parent of dir_s1d3.  Granting MAKE_REG only
on dir_s1d3 allows renaming into dir_s1d3, which inherits REFER from
dir_s1d2 when walking up the hierarchy.  However, dir_s1d2 itself lacks
MAKE_REG, so test_rename(file1_s1d3, file1_s1d2) and
test_exchange(file2_s1d2, file2_s1d3) both fail with EACCES in
current_check_refer_path().

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