Message-ID: <af464773-b01b-f3a4-474d-0efb2cfae142@huawei-partners.com>
Date: Sat, 22 Nov 2025 14:13:08 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack3000@...il.com>
CC: <mic@...ikod.net>, <gnoack@...gle.com>, <willemdebruijn.kernel@...il.com>,
	<matthieu@...fet.re>, <linux-security-module@...r.kernel.org>,
	<netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>,
	<yusongping@...wei.com>, <artem.kuzin@...wei.com>,
	<konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v4 01/19] landlock: Support socket access-control

On 11/22/2025 1:49 PM, Günther Noack wrote:
> Hello!
> 
> On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
>> It is possible to create sockets of the same protocol with different
>> protocol number values. For example, TCP sockets can be created using one
>> of the following commands:
>>      1. fd = socket(AF_INET, SOCK_STREAM, 0);
>>      2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
>> protocol of the given protocol family and can be mapped to another
>> value.
>>
>> Socket rules do not perform such mappings to not increase complexity
>> of rules definition and their maintenance.
> 
> Minor phrasing nit: Maybe we can phrase this constructively, like
> "rules operate on the socket(2) parameters as they are passed by the
> user, before this mapping happens"?

OK, thats sounds good.

> 
> 
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f030adc462ee..030c96cb5d25 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
>>   	 * flags`_).
>>   	 */
>>   	__u64 handled_access_net;
>> +	/**
>> +	 * @handled_access_socket: Bitmask of handled actions performed on sockets
>> +	 * (cf. `Socket flags`).
>> +	 */
>> +	__u64 handled_access_socket;
> 
> This struct can only be extended at the end, for ABI compatibility reasons.
> 
> In the call to landlock_create_ruleset(2), the user passes the __user
> pointer to this struct along with its size (as known to the user at
> compile time).  When we copy this into the kernel, we blank out the
> struct and only copy the prefix of the caller-supplied size.  The
> implementation is in copy_min_struct_from_user() in landlock/syscalls.c.

Indeed... Thanks for pointing on this, I'll move this field in the end
of the structure.

> 
> When you rearrange the order, please also update it in other places
> where these fields are mentioned next to each other, for
> consistency. I'll try to point it out where I see it in the review,
> but I might miss some places.

ok

> 
>>   	/**
>>   	 * @scoped: Bitmask of scopes (cf. `Scope flags`_)
>>   	 * restricting a Landlock domain from accessing outside
>> @@ -140,6 +145,11 @@ enum landlock_rule_type {
>>   	 * landlock_net_port_attr .
>>   	 */
>>   	LANDLOCK_RULE_NET_PORT,
>> +	/**
>> +	 * @LANDLOCK_RULE_SOCKET: Type of a &struct
>> +	 * landlock_socket_attr.
>                                 ^
> 
> Nit: Adjacent documentation has a space before the dot.
> I assume this is needed for kernel doc formatting?

Probably, I'll fix this anyway.

> 
>> +	 */
>> +	LANDLOCK_RULE_SOCKET,
>>   };
>>   
>>   /**
>> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
>>   	__u64 port;
>>   };
>>   
>> +/**
>> + * struct landlock_socket_attr - Socket protocol definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_socket_attr {
>> +	/**
>> +	 * @allowed_access: Bitmask of allowed access for a socket protocol
>> +	 * (cf. `Socket flags`_).
>> +	 */
>> +	__u64 allowed_access;
>> +	/**
>> +	 * @family: Protocol family used for communication
>> +	 * (cf. include/linux/socket.h).
>> +	 */
>> +	__s32 family;
>> +	/**
>> +	 * @type: Socket type (cf. include/linux/net.h)
>> +	 */
>> +	__s32 type;
>> +	/**
>> +	 * @protocol: Communication protocol specific to protocol family set in
>> +	 * @family field.
> 
> This is specific to both the @family and the @type, not just the @family.
> 
>>>From socket(2):
> 
>    Normally only a single protocol exists to support a particular
>    socket type within a given protocol family.
> 
> For instance, in your commit message above the protocol in the example
> is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
> with SOCK_DGRAM.

You're right.

> 
>> +	 */
>> +	__s32 protocol;
>> +} __attribute__((packed));
> 
> Since we are in the UAPI header, please also document the wildcard
> values for @type and @protocol.

I'll add the description, thanks!

> 
> (Remark, should those be exposed as constants?)

I thought it could overcomplicate socket rules definition and Landlock
API. Do you think introducing such constants will be better decision?

> 
> 
>> diff --git a/security/landlock/access.h b/security/landlock/access.h
>> index 7961c6630a2d..03ccd6fbfe83 100644
>> --- a/security/landlock/access.h
>> +++ b/security/landlock/access.h
>> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>>   /* Makes sure all network access rights can be stored. */
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>> +/* Makes sure all socket access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
>>   /* Makes sure all scoped rights can be stored. */
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
>>   /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>>   struct access_masks {
>>   	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
>>   	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
>> +	access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
>>   	access_mask_t scope : LANDLOCK_NUM_SCOPE;
> 
> (Please re-adjust field order for consistency with UAPI)

ok, will be fixed in all such places.

> 
>>   };
> 
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index dfcdc19ea268..a34d2dbe3954 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   	return new_ruleset;
>>   }
>>   
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask,
>> -			const access_mask_t net_access_mask,
>> -			const access_mask_t scope_mask)
>> +struct landlock_ruleset *landlock_create_ruleset(
>> +	const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
>> +	const access_mask_t socket_access_mask, const access_mask_t scope_mask)
> 
> (Please re-adjust field order for consistency with UAPI)
> 
>>   {
>>   	struct landlock_ruleset *new_ruleset;
>>   
>>   	/* Informs about useless ruleset. */
>> -	if (!fs_access_mask && !net_access_mask && !scope_mask)
>> +	if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
>> +	    !scope_mask)
> 
> (Please re-adjust field order for consistency with UAPI)
> 
>>   		return ERR_PTR(-ENOMSG);
>>   	new_ruleset = create_ruleset(1);
>>   	if (IS_ERR(new_ruleset))
>> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>>   		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>>   	if (net_access_mask)
>>   		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> +	if (socket_access_mask)
>> +		landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
>> +						0);
> 
> (Please re-adjust order of these "if"s for consistency with UAPI)
> 
>>   	if (scope_mask)
>>   		landlock_add_scope_mask(new_ruleset, scope_mask, 0);
>>   	return new_ruleset;
> 
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 1a78cba662b2..a60ede2fc2a5 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -189,10 +204,9 @@ struct landlock_ruleset {
>>   	};
>>   };
>>   
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t access_mask_fs,
>> -			const access_mask_t access_mask_net,
>> -			const access_mask_t scope_mask);
>> +struct landlock_ruleset *landlock_create_ruleset(
>> +	const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
>> +	const access_mask_t access_mask_socket, const access_mask_t scope_mask);
> 
> (Please re-adjust field order for consistency with UAPI)
> 
>> index 000000000000..28a80dcad629
>> --- /dev/null
>> +++ b/security/landlock/socket.c
>> @@ -0,0 +1,105 @@
>> [...]
>> +#define TYPE_ALL (-1)
>> +#define PROTOCOL_ALL (-1)
> 
> Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?

answered above.

> 
> 
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 33eafb71e4f3..e9f500f97c86 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -101,9 +104,10 @@ static void build_check_abi(void)
>>   	 */
>>   	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>>   	ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> +	ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>>   	ruleset_size += sizeof(ruleset_attr.scoped);
> (Please re-adjust field order for consistency with UAPI)
> 
>>   	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> -	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>> +	BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
>> [...]
> 
>> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>   	    LANDLOCK_MASK_ACCESS_NET)
>>   		return -EINVAL;
>>   
>> +	/* Checks socket content (and 32-bits cast). */
>> +	if ((ruleset_attr.handled_access_socket |
>> +	     LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
>> +		return -EINVAL;
>> +
>>   	/* Checks IPC scoping content (and 32-bits cast). */
>>   	if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
>>   		return -EINVAL;
>> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>   	/* Checks arguments and transforms to kernel struct. */
>>   	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>>   					  ruleset_attr.handled_access_net,
>> +					  ruleset_attr.handled_access_socket,
>>   					  ruleset_attr.scoped);
> 
> (Please re-adjust field order for consistency with UAPI)
> 
>>   	if (IS_ERR(ruleset))
>>   		return PTR_ERR(ruleset);
>> [...]
> 
>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>>    *   &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>>    *   handled accesses)
>>    * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>> + *   &landlock_socket_attr.protocol is greater than 65534;
> 
> Hmm, this is a bit annoying that these values have such unusual
> bounds, even though the input parameters are 32 bit.  We are exposing
> a little bit that we are internally storing this with only 8 and 16
> bits...  (I don't know a better solution immediately either, though. I
> think we discussed this on a previous version of the patch set as well
> and ended up with permitting larger values than the narrower SOCK_MAX
> etc bounds.)

I agree, one of the possible solutions may be to store larger values in
socket keys (eg. s32), but this would require to make a separate
interface for storing socket rules (in order to not change key size for
other type of rules which is currently 32-64 bit depending on virtual
address size).

> 
>>    * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
>>    *   0);
>>    * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
>> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>>   		return add_rule_path_beneath(ruleset, rule_attr);
>>   	case LANDLOCK_RULE_NET_PORT:
>>   		return add_rule_net_port(ruleset, rule_attr);
>> +	case LANDLOCK_RULE_SOCKET:
>> +		return add_rule_socket(ruleset, rule_attr);
>>   	default:
>>   		return -EINVAL;
>>   	}
> 
> –Günther

Powered by blists - more mailing lists