Re: [RFC PATCH v2 1/9] landlock: Refactor current_check_access_socket() access right check

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

Hello!

Thanks for sending round 2 of this patch set!

On Wed, Aug 14, 2024 at 11:01:43AM +0800, Mikhail Ivanov wrote:
> The current_check_access_socket() function contains a set of address
> validation checks for bind(2) and connect(2) hooks. Separate them from
> an actual port access right checking. It is required for the (future)
> hooks that do not perform address validation.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  security/landlock/net.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index c8bcd29bde09..669ba260342f 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -2,7 +2,7 @@
>  /*
>   * Landlock LSM - Network management and hooks
>   *
> - * Copyright © 2022-2023 Huawei Tech. Co., Ltd.
> + * Copyright © 2022-2024 Huawei Tech. Co., Ltd.
>   * Copyright © 2022-2023 Microsoft Corporation
>   */
>  
> @@ -61,17 +61,34 @@ static const struct landlock_ruleset *get_current_net_domain(void)
>  	return dom;
>  }
>  
> -static int current_check_access_socket(struct socket *const sock,
> -				       struct sockaddr *const address,
> -				       const int addrlen,
> -				       access_mask_t access_request)
> +static int check_access_socket(const struct landlock_ruleset *const dom,
> +			       __be16 port, access_mask_t access_request)

It might be worth briefly spelling out in documentation that access_request in
current_check_access_socket() may only have a single bit set.  This is different
to other places where access_mask_t is used, where combinations of these flags
are possible.

These functions do checks for special cases using "if (access_request ==
LANDLOCK_ACCESS_NET_CONNECT_TCP)" and the same for "bind".  I think it's a
reasonable way to simplify the implementation here, but we have to be careful to
not accidentally use it differently.

It is a preexisting issue, so I don't consider it a blocker, but it might be
worth fixing while we are at it?


>  {
> -	__be16 port;
>  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>  	const struct landlock_rule *rule;
>  	struct landlock_id id = {
>  		.type = LANDLOCK_KEY_NET_PORT,
>  	};
> +
> +	id.key.data = (__force uintptr_t)port;
> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> +
> +	rule = landlock_find_rule(dom, id);
> +	access_request = landlock_init_layer_masks(
> +		dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
> +	if (landlock_unmask_layers(rule, access_request, &layer_masks,
> +				   ARRAY_SIZE(layer_masks)))
> +		return 0;
> +
> +	return -EACCES;
> +}
> +
> +static int current_check_access_socket(struct socket *const sock,

Re-reading the implementation of this function, it was surprised how specialized
it is towards the "connect" and "bind" use cases, which it has specific code
paths for.  This does not look like it would extend naturally to additional
operations.

After your refactoring, current_check_access_socket() is now (a) checking that
we are looking at a TCP address, and extracting the port, and then (b) doing
connect- and bind-specific logic, and then (c) calling check_access_socket().

Would it maybe be possible to turn the code logic around by creating a
"get_tcp_port()" helper function for step (a), and then doing all of (a), (b)
and (c) directly from hook_socket_bind() and hook_socket_connect()?  It would
have the upside that in step (b) you don't need to distinguish between bind and
connect because it would be clear from the context which of the two cases we are
in.  It would also remove the need for a function that only supports one bit in
the access_mask_t, which is potentially surprising.

Thanks,
—Günther