Re: [RFC PATCH v3 06/19] selftests/landlock: Test adding a rule for unhandled access

["Günther Noack" <gnoack@google.com>]

Hi!

On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote:
> Add test that validates behaviour of Landlock after rule with
> unhandled access is added.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> Changes since v2:
> * Replaces EXPECT_EQ with ASSERT_EQ for close().
> * Refactors commit title and message.
> 
> Changes since v1:
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 811bdaa95a7a..d2fedfca7193 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access)
>  	ASSERT_EQ(0, close(ruleset_fd));
>  }
>  
> +TEST_F(protocol, rule_with_unhandled_access)
> +{
> +	struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	struct landlock_socket_attr protocol = {
> +		.family = self->prot.family,
> +		.type = self->prot.type,
> +	};
> +	int ruleset_fd;
> +	__u64 access;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	for (access = 1; access > 0; access <<= 1) {
> +		int err;
> +
> +		protocol.allowed_access = access;
> +		err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					&protocol, 0);
> +		if (access == ruleset_attr.handled_access_socket) {
> +			EXPECT_EQ(0, err);
> +		} else {
> +			EXPECT_EQ(-1, err);
> +			EXPECT_EQ(EINVAL, errno);
> +		}
> +	}
> +
> +	ASSERT_EQ(0, close(ruleset_fd));
> +}
> +

I should probably have noticed this on the first review round; you are not
actually exercising any scenario here where a rule with unhandled access is
added.

To clarify, the notion of an access right being "unhandled" means that the
access right was not listed at ruleset creation time in the ruleset_attr's
.handled_access_* field where it would have belonged.  If that is the case,
adding a ruleset with that access right is going to be denied.

As an example:
If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else,
then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE,
that call is supposed to fail -- because the "socket creation" access right is
not handled.

IMHO the test would become more reasonable if it was more clearly "handling"
something entirely unrelated at ruleset creation time, e.g. one of the file
system access rights.  (And we could do the same for the "net" and "fs" tests as
well.)

Your test is a copy of the same test for the "net" rights, which in turn is a
copy of teh same test for the "fs" rights.  When the "fs" test was written, the
"fs" access rights were the only ones that could be used at all to create a
ruleset, but this is not true any more.

—Günther