[PATCH] landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER

[PATCH] landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER

[pandaWall]

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.
---
 security/landlock/domain.h                 | 15 +++++++--
 tools/testing/selftests/landlock/fs_test.c | 37 ++++++++++++++++++++++
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/security/landlock/domain.h b/security/landlock/domain.h
index 935e18b12..07d4a23fc 100644
--- a/security/landlock/domain.h
+++ b/security/landlock/domain.h
@@ -262,9 +262,18 @@ static inline access_mask_t
 landlock_get_fs_access_mask(const struct landlock_domain *const domain,
 			    const u16 layer_level)
 {
-	/* Handles all initially denied by default access rights. */
-	return domain->layers[layer_level].handled.fs |
-	       _LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
+	access_mask_t handled = domain->layers[layer_level].handled.fs;
+
+	/*
+	 * Only layers that handle filesystem access rights implicitly
+	 * handle REFER.  A layer that handles other rule types (e.g.
+	 * network, scopes) must not imply REFER, otherwise it would
+	 * spuriously deny link and rename operations.
+	 */
+	if (handled)
+		handled |= _LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
+
+	return handled;
 }
 
 static inline access_mask_t
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index f725882fa..5588f0ee5 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));
+}
+
 /*
  * Tests precedence over renames: denied by default for different parent
  * directories, *with* a rule matching a parent directory, but not directly
-- 
2.56.0

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

[sashiko-bot]

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

[RFC PATCH v2 1/2] landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER

[pandaWall]

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>
---
 security/landlock/domain.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/security/landlock/domain.h b/security/landlock/domain.h
index 935e18b12..07d4a23fc 100644
--- a/security/landlock/domain.h
+++ b/security/landlock/domain.h
@@ -262,9 +262,18 @@ static inline access_mask_t
 landlock_get_fs_access_mask(const struct landlock_domain *const domain,
 			    const u16 layer_level)
 {
-	/* Handles all initially denied by default access rights. */
-	return domain->layers[layer_level].handled.fs |
-	       _LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
+	access_mask_t handled = domain->layers[layer_level].handled.fs;
+
+	/*
+	 * Only layers that handle filesystem access rights implicitly
+	 * handle REFER.  A layer that handles other rule types (e.g.
+	 * network, scopes) must not imply REFER, otherwise it would
+	 * spuriously deny link and rename operations.
+	 */
+	if (handled)
+		handled |= _LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
+
+	return handled;
 }
 
 static inline access_mask_t
-- 
2.56.0

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

[pandaWall]

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>
---
 tools/testing/selftests/landlock/fs_test.c | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index f725882fa..cf1128163 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);
+	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));
+}
+
 /*
  * Tests precedence over renames: denied by default for different parent
  * directories, *with* a rule matching a parent directory, but not directly
-- 
2.56.0

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

[sashiko-bot]

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

Re: [RFC PATCH v2 1/2] landlock: Fix layer composition of LANDLOCK_ACCESS_FS_REFER

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] landlock: Signed-off-by trailer uses handle instead of real
  human name
--

commit 6fdefb3a96cf87892d81d37297b85c7906fe0143
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>

[Severity: High]
The Signed-off-by trailer appears to use a handle ("pandaWall") rather than
a real full name.  Per Documentation/process/submitting-patches.rst, the
Developer's Certificate of Origin requires a real name and does not allow
pseudonyms or handles.

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