| Message ID | 20240524093015.2402952-4-ivanov.mikhail1@huawei-partners.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Socket type control for Landlock | expand |
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
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 --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
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