diff mbox series

[RFC,v2,03/12] selftests/landlock: Add protocol.create to socket tests

Message ID 20240524093015.2402952-4-ivanov.mikhail1@huawei-partners.com (mailing list archive)
State RFC
Headers show
Series Socket type control for Landlock | expand

Checks

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

Commit Message

Mikhail Ivanov May 24, 2024, 9:30 a.m. UTC 
Initiate socket_test.c selftests. Add protocol fixture for tests
with changeable family-type values. Only most common variants of
protocols (like ipv4-tcp,ipv6-udp, unix) were added.
Add simple socket access right checking test.

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

Changes since v1:
* Replaces test_socket_create() and socket_variant() helpers
  with test_socket().
* Renames domain to family in protocol fixture.
* Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
  check AF_UNSPEC socket creation case.
* Formats code with clang-format.
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
 1 file changed, 181 insertions(+)
 create mode 100644 tools/testing/selftests/landlock/socket_test.c

Comments

Günther Noack May 27, 2024, 3:27 p.m. UTC | #1 
On Fri, May 24, 2024 at 05:30:06PM +0800, Mikhail Ivanov wrote:
> Initiate socket_test.c selftests. Add protocol fixture for tests
> with changeable family-type values. Only most common variants of
> protocols (like ipv4-tcp,ipv6-udp, unix) were added.
> Add simple socket access right checking test.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Replaces test_socket_create() and socket_variant() helpers
>   with test_socket().
> * Renames domain to family in protocol fixture.
> * Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
>   check AF_UNSPEC socket creation case.
> * Formats code with clang-format.
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 tools/testing/selftests/landlock/socket_test.c
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> new file mode 100644
> index 000000000000..4c51f89ed578
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock tests - Socket
> + *
> + * Copyright © 2024 Huawei Tech. Co., Ltd.
> + * Copyright © 2024 Microsoft Corporation

It looked to me like these patches came from Huawei?
Was this left by accident?


> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <errno.h>
> +#include <linux/landlock.h>
> +#include <sched.h>
> +#include <string.h>
> +#include <sys/prctl.h>
> +#include <sys/socket.h>
> +
> +#include "common.h"
> +
> +/* clang-format off */
> +
> +#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
> +
> +#define ACCESS_ALL ( \
> +	LANDLOCK_ACCESS_SOCKET_CREATE)
> +
> +/* clang-format on */

It does not look like clang-format would really mess up this format in a bad
way.  Maybe we can remove the "clang-format off" section here and just write the
"#define"s on one line?

ACCESS_ALL is unused in this commit.
Should it be introduced in a subsequent commit instead?


> +static int test_socket(const struct service_fixture *const srv)
> +{
> +	int fd;
> +
> +	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
> +	if (fd < 0)
> +		return errno;
> +	/*
> +	 * Mixing error codes from close(2) and socket(2) should not lead to any
> +	 * (access type) confusion for this test.
> +	 */
> +	if (close(fd) != 0)
> +		return errno;
> +	return 0;
> +}

I personally find that it helps me remember if these test helpers have the same
signature as the syscall that they are exercising.  (But I don't feel very
strongly about it.  Just a suggestion.)


> [...]
>
> +TEST_F(protocol, create)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	const struct landlock_socket_attr create_socket_attr = {
> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +
> +	int ruleset_fd;
> +
> +	/* Allowed create */
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +				       &create_socket_attr, 0));
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	ASSERT_EQ(0, test_socket(&self->srv0));
> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
> +
> +	/* Denied create */
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	ASSERT_EQ(EACCES, test_socket(&self->srv0));
> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));

Should we exhaustively try out the other combinations (other than selv->srv0)
here?  I assume socket() should always fail for these?

(If you are alredy doing this in another commit that I have not looked at yet,
please ignore this comment.)

—Günther
Mikhail Ivanov May 30, 2024, 12:50 p.m. UTC | #2 
5/27/2024 6:27 PM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:06PM +0800, Mikhail Ivanov wrote:
>> Initiate socket_test.c selftests. Add protocol fixture for tests
>> with changeable family-type values. Only most common variants of
>> protocols (like ipv4-tcp,ipv6-udp, unix) were added.
>> Add simple socket access right checking test.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Replaces test_socket_create() and socket_variant() helpers
>>    with test_socket().
>> * Renames domain to family in protocol fixture.
>> * Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
>>    check AF_UNSPEC socket creation case.
>> * Formats code with clang-format.
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
>>   1 file changed, 181 insertions(+)
>>   create mode 100644 tools/testing/selftests/landlock/socket_test.c
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> new file mode 100644
>> index 000000000000..4c51f89ed578
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock tests - Socket
>> + *
>> + * Copyright © 2024 Huawei Tech. Co., Ltd.
>> + * Copyright © 2024 Microsoft Corporation
> 
> It looked to me like these patches came from Huawei?
> Was this left by accident?

Yeah, second line should be removed. Thanks!

> 
> 
>> + */
>> +
>> +#define _GNU_SOURCE
>> +
>> +#include <errno.h>
>> +#include <linux/landlock.h>
>> +#include <sched.h>
>> +#include <string.h>
>> +#include <sys/prctl.h>
>> +#include <sys/socket.h>
>> +
>> +#include "common.h"
>> +
>> +/* clang-format off */
>> +
>> +#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
>> +
>> +#define ACCESS_ALL ( \
>> +	LANDLOCK_ACCESS_SOCKET_CREATE)
>> +
>> +/* clang-format on */
> 
> It does not look like clang-format would really mess up this format in a bad
> way.  Maybe we can remove the "clang-format off" section here and just write the
> "#define"s on one line?

You're right, I'll fix it

> 
> ACCESS_ALL is unused in this commit.
> Should it be introduced in a subsequent commit instead?

Indeed, thanks

> 
> 
>> +static int test_socket(const struct service_fixture *const srv)
>> +{
>> +	int fd;
>> +
>> +	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
>> +	if (fd < 0)
>> +		return errno;
>> +	/*
>> +	 * Mixing error codes from close(2) and socket(2) should not lead to any
>> +	 * (access type) confusion for this test.
>> +	 */
>> +	if (close(fd) != 0)
>> +		return errno;
>> +	return 0;
>> +}
> 
> I personally find that it helps me remember if these test helpers have the same
> signature as the syscall that they are exercising.  (But I don't feel very
> strongly about it.  Just a suggestion.)

You're right, in this case test_socket() would be more clear.
I'll fix it.

> 
> 
>> [...]
>>
>> +TEST_F(protocol, create)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +	};
>> +	const struct landlock_socket_attr create_socket_attr = {
>> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +
>> +	int ruleset_fd;
>> +
>> +	/* Allowed create */
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +				       &create_socket_attr, 0));
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	EXPECT_EQ(0, close(ruleset_fd));
>> +
>> +	ASSERT_EQ(0, test_socket(&self->srv0));
>> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>> +
>> +	/* Denied create */
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	EXPECT_EQ(0, close(ruleset_fd));
>> +
>> +	ASSERT_EQ(EACCES, test_socket(&self->srv0));
>> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
> 
> Should we exhaustively try out the other combinations (other than selv->srv0)
> here?  I assume socket() should always fail for these?

Do you mean testing all supported protocols? AFAICS this will require
adding ~80 FIXTURE_VARIANTs, but it won't be an issue if you think that
it can be useful.

> 
> (If you are alredy doing this in another commit that I have not looked at yet,
> please ignore this comment.)
> 
> —Günther
diff mbox series

Patch

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
new file mode 100644
index 000000000000..4c51f89ed578
--- /dev/null
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -0,0 +1,181 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock tests - Socket
+ *
+ * Copyright © 2024 Huawei Tech. Co., Ltd.
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <linux/landlock.h>
+#include <sched.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <sys/socket.h>
+
+#include "common.h"
+
+/* clang-format off */
+
+#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
+
+#define ACCESS_ALL ( \
+	LANDLOCK_ACCESS_SOCKET_CREATE)
+
+/* clang-format on */
+
+struct protocol_variant {
+	int family;
+	int type;
+};
+
+struct service_fixture {
+	struct protocol_variant protocol;
+};
+
+static void setup_namespace(struct __test_metadata *const _metadata)
+{
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0, unshare(CLONE_NEWNET));
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+static int test_socket(const struct service_fixture *const srv)
+{
+	int fd;
+
+	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
+	if (fd < 0)
+		return errno;
+	/*
+	 * Mixing error codes from close(2) and socket(2) should not lead to any
+	 * (access type) confusion for this test.
+	 */
+	if (close(fd) != 0)
+		return errno;
+	return 0;
+}
+
+FIXTURE(protocol)
+{
+	struct service_fixture srv0, unspec_srv0;
+};
+
+FIXTURE_VARIANT(protocol)
+{
+	const struct protocol_variant protocol;
+};
+
+FIXTURE_SETUP(protocol)
+{
+	const struct protocol_variant prot_unspec = {
+		.family = AF_UNSPEC,
+		.type = SOCK_STREAM,
+	};
+
+	disable_caps(_metadata);
+	self->srv0.protocol = variant->protocol;
+	self->unspec_srv0.protocol = prot_unspec;
+	setup_namespace(_metadata);
+};
+
+FIXTURE_TEARDOWN(protocol)
+{
+}
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_stream) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_UNIX,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_dgram) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_UNIX,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_udp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET6,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_udp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET6,
+		.type = SOCK_DGRAM,
+	},
+};
+
+TEST_F(protocol, create)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	const struct landlock_socket_attr create_socket_attr = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+
+	int ruleset_fd;
+
+	/* Allowed create */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &create_socket_attr, 0));
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	ASSERT_EQ(0, test_socket(&self->srv0));
+	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
+
+	/* Denied create */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	ASSERT_EQ(EACCES, test_socket(&self->srv0));
+	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
+}
+
+TEST_HARNESS_MAIN