diff mbox series

[RFC,v2,5/9] selftests/landlock: Test listen on connected socket

Message ID 20240814030151.2380280-6-ivanov.mikhail1@huawei-partners.com (mailing list archive)
State RFC
Headers show
Series Support TCP listen access-control | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async

Commit Message

Mikhail Ivanov Aug. 14, 2024, 3:01 a.m. UTC 
Test checks that listen(2) doesn't wrongfully return -EACCES instead
of -EINVAL when trying to listen for an incorrect socket state.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Uses 'protocol' fixture instead of 'ipv4_tcp'.
* Minor fixes.
---
 tools/testing/selftests/landlock/net_test.c | 74 +++++++++++++++++++++
 1 file changed, 74 insertions(+)

Comments

Günther Noack Aug. 20, 2024, 1:01 p.m. UTC | #1 
On Wed, Aug 14, 2024 at 11:01:47AM +0800, Mikhail Ivanov wrote:
> Test checks that listen(2) doesn't wrongfully return -EACCES instead
> of -EINVAL when trying to listen for an incorrect socket state.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Uses 'protocol' fixture instead of 'ipv4_tcp'.
> * Minor fixes.
> ---
>  tools/testing/selftests/landlock/net_test.c | 74 +++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index b6fe9bde205f..551891b18b7a 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -926,6 +926,80 @@ TEST_F(protocol, connect_unspec)
>  	EXPECT_EQ(0, close(bind_fd));
>  }
>  
> +TEST_F(protocol, listen_on_connected)
> +{
> +	int bind_fd, status;
> +	pid_t child;
> +
> +	if (variant->sandbox == TCP_SANDBOX) {
> +		const struct landlock_ruleset_attr ruleset_attr = {
> +			.handled_access_net = ACCESS_ALL,
> +		};
> +		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
> +			.allowed_access = ACCESS_ALL,
> +			.port = self->srv0.port,
> +		};
> +		const struct landlock_net_port_attr tcp_denied_listen_p1 = {
> +			.allowed_access = ACCESS_ALL &
> +					  ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
> +			.port = self->srv1.port,
> +		};
> +		int ruleset_fd;
> +
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Allows all actions for the first port. */
> +		ASSERT_EQ(0,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +					    &tcp_not_restricted_p0, 0));
> +
> +		/* Denies listening for the second port. */
> +		ASSERT_EQ(0,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +					    &tcp_denied_listen_p1, 0));
> +
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}

Same remarks as in the previous commit apply here as well:

  - The if condition does the same thing, can maybe be deduplicated.
  - Can merge ruleset_fd declaration and assignment into one line.
    (This happens in a few more tests in later commits as well,
    please double check these as well.)

> +
> +	if (variant->prot.type != SOCK_STREAM)
> +		SKIP(return, "listen(2) is supported only on stream sockets");
> +
> +	/* Initializes listening socket. */
> +	bind_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, bind_fd);
> +	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
> +	EXPECT_EQ(0, listen_variant(bind_fd, backlog));

I believe if bind() or listen() fail here, it does not make sense to continue
the test execution, so ASSERT_EQ would be more appropriate than EXPECT_EQ.


> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		int connect_fd;
> +
> +		/* Closes listening socket for the child. */
> +		EXPECT_EQ(0, close(bind_fd));

You don't need to do this from a child process, you can just connect() from the
same process to the listening port.  (Since you are not calling accept(), the
server won't pick up the phone on the other end, but that is still enough to
connect successfully.)  It would simplify the story of correctly propagating
test exit statuses as well.

> +
> +		connect_fd = socket_variant(&self->srv1);
> +		ASSERT_LE(0, connect_fd);
> +		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
> +
> +		/* Tries to listen on connected socket. */
> +		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));

Since this assertion is the actual point of the test,
maybe we could emphasize it a bit more with a comment here?

e.g:

/*
 * Checks that we always return EINVAL
 * and never accidentally return EACCES, if listen(2) fails.
 */

> +
> +		EXPECT_EQ(0, close(connect_fd));
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +
> +	EXPECT_EQ(child, waitpid(child, &status, 0));
> +	EXPECT_EQ(1, WIFEXITED(status));
> +	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	EXPECT_EQ(0, close(bind_fd));
> +}
> +
>  FIXTURE(ipv4)
>  {
>  	struct service_fixture srv0, srv1;
> -- 
> 2.34.1
>
Mikhail Ivanov Aug. 20, 2024, 1:42 p.m. UTC | #2 
8/20/2024 4:01 PM, Günther Noack wrote:
> On Wed, Aug 14, 2024 at 11:01:47AM +0800, Mikhail Ivanov wrote:
>> Test checks that listen(2) doesn't wrongfully return -EACCES instead
>> of -EINVAL when trying to listen for an incorrect socket state.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Uses 'protocol' fixture instead of 'ipv4_tcp'.
>> * Minor fixes.
>> ---
>>   tools/testing/selftests/landlock/net_test.c | 74 +++++++++++++++++++++
>>   1 file changed, 74 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
>> index b6fe9bde205f..551891b18b7a 100644
>> --- a/tools/testing/selftests/landlock/net_test.c
>> +++ b/tools/testing/selftests/landlock/net_test.c
>> @@ -926,6 +926,80 @@ TEST_F(protocol, connect_unspec)
>>   	EXPECT_EQ(0, close(bind_fd));
>>   }
>>   
>> +TEST_F(protocol, listen_on_connected)
>> +{
>> +	int bind_fd, status;
>> +	pid_t child;
>> +
>> +	if (variant->sandbox == TCP_SANDBOX) {
>> +		const struct landlock_ruleset_attr ruleset_attr = {
>> +			.handled_access_net = ACCESS_ALL,
>> +		};
>> +		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
>> +			.allowed_access = ACCESS_ALL,
>> +			.port = self->srv0.port,
>> +		};
>> +		const struct landlock_net_port_attr tcp_denied_listen_p1 = {
>> +			.allowed_access = ACCESS_ALL &
>> +					  ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
>> +			.port = self->srv1.port,
>> +		};
>> +		int ruleset_fd;
>> +
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Allows all actions for the first port. */
>> +		ASSERT_EQ(0,
>> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>> +					    &tcp_not_restricted_p0, 0));
>> +
>> +		/* Denies listening for the second port. */
>> +		ASSERT_EQ(0,
>> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>> +					    &tcp_denied_listen_p1, 0));
>> +
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
> 
> Same remarks as in the previous commit apply here as well:
> 
>    - The if condition does the same thing, can maybe be deduplicated.
>    - Can merge ruleset_fd declaration and assignment into one line.
>      (This happens in a few more tests in later commits as well,
>      please double check these as well.)

Thanks for mentioning! You can check my reply in the previous commit
discussion.

> 
>> +
>> +	if (variant->prot.type != SOCK_STREAM)
>> +		SKIP(return, "listen(2) is supported only on stream sockets");
>> +
>> +	/* Initializes listening socket. */
>> +	bind_fd = socket_variant(&self->srv0);
>> +	ASSERT_LE(0, bind_fd);
>> +	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
>> +	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
> 
> I believe if bind() or listen() fail here, it does not make sense to continue
> the test execution, so ASSERT_EQ would be more appropriate than EXPECT_EQ.

Will be fixed, thanks.

> 
> 
>> +
>> +	child = fork();
>> +	ASSERT_LE(0, child);
>> +	if (child == 0) {
>> +		int connect_fd;
>> +
>> +		/* Closes listening socket for the child. */
>> +		EXPECT_EQ(0, close(bind_fd));
> 
> You don't need to do this from a child process, you can just connect() from the
> same process to the listening port.  (Since you are not calling accept(), the
> server won't pick up the phone on the other end, but that is still enough to
> connect successfully.)  It would simplify the story of correctly propagating
> test exit statuses as well.

Thanks, I'll fix this.

> 
>> +
>> +		connect_fd = socket_variant(&self->srv1);
>> +		ASSERT_LE(0, connect_fd);
>> +		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
>> +
>> +		/* Tries to listen on connected socket. */
>> +		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));
> 
> Since this assertion is the actual point of the test,
> maybe we could emphasize it a bit more with a comment here?
> 
> e.g:
> 
> /*
>   * Checks that we always return EINVAL
>   * and never accidentally return EACCES, if listen(2) fails.
>   */

You're right.. current description doesn't give an understanding of why
this test is needed at all. I'll change it.

> 
>> +
>> +		EXPECT_EQ(0, close(connect_fd));
>> +		_exit(_metadata->exit_code);
>> +		return;
>> +	}
>> +
>> +	EXPECT_EQ(child, waitpid(child, &status, 0));
>> +	EXPECT_EQ(1, WIFEXITED(status));
>> +	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
>> +
>> +	EXPECT_EQ(0, close(bind_fd));
>> +}
>> +
>>   FIXTURE(ipv4)
>>   {
>>   	struct service_fixture srv0, srv1;
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index b6fe9bde205f..551891b18b7a 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -926,6 +926,80 @@  TEST_F(protocol, connect_unspec)
 	EXPECT_EQ(0, close(bind_fd));
 }
 
+TEST_F(protocol, listen_on_connected)
+{
+	int bind_fd, status;
+	pid_t child;
+
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = ACCESS_ALL,
+		};
+		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
+			.allowed_access = ACCESS_ALL,
+			.port = self->srv0.port,
+		};
+		const struct landlock_net_port_attr tcp_denied_listen_p1 = {
+			.allowed_access = ACCESS_ALL &
+					  ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
+			.port = self->srv1.port,
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Allows all actions for the first port. */
+		ASSERT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_not_restricted_p0, 0));
+
+		/* Denies listening for the second port. */
+		ASSERT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_denied_listen_p1, 0));
+
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	if (variant->prot.type != SOCK_STREAM)
+		SKIP(return, "listen(2) is supported only on stream sockets");
+
+	/* Initializes listening socket. */
+	bind_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, bind_fd);
+	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
+	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		int connect_fd;
+
+		/* Closes listening socket for the child. */
+		EXPECT_EQ(0, close(bind_fd));
+
+		connect_fd = socket_variant(&self->srv1);
+		ASSERT_LE(0, connect_fd);
+		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
+
+		/* Tries to listen on connected socket. */
+		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));
+
+		EXPECT_EQ(0, close(connect_fd));
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	EXPECT_EQ(child, waitpid(child, &status, 0));
+	EXPECT_EQ(1, WIFEXITED(status));
+	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+	EXPECT_EQ(0, close(bind_fd));
+}
+
 FIXTURE(ipv4)
 {
 	struct service_fixture srv0, srv1;