[RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Mikhail Ivanov]

5/27/2024 11:48 AM, Günther Noack wrote:
> Hello Mikhail!
> 
> Thanks for sending another revision of this patch set!
> 
> On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
>> Add hook to security_socket_post_create(), which checks whether the socket
>> type and family are allowed by domain. Hook is called after initializing
>> the socket in the network stack to not wrongfully return EACCES for a
>> family-type pair, which is considered invalid by the protocol.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1 at huawei-partners.com>
> 
> ## Some observations that *do not* need to be addressed in this commit, IMHO:
> 
> get_raw_handled_socket_accesses, get_current_socket_domain and
> current_check_access_socket are based on the similarly-named functions from
> net.c (and fs.c), and it makes sense to stay consistent with these.
> 
> There are some possible refactorings that could maybe be applied to that code,
> but given that the same ones would apply to net.c as well, it's probably best to
> address these separately.
> 
>    * Should get_raw_handled_socket_accesses be inlined
It's a fairly simple and compact function, so compiler should inline it
without any problems. Mickaël was against optional inlines [1].

[1] 
https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/

>    * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?

Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1
(see get_*_domain functions).

>    * Can we refactor out commonalities (probably not worth it right now though)?

I had a few ideas about refactoring commonalities, as currently landlock
has several repetitive patterns in the code. But solution requires a
good design and a separate patch. Probably it's worth opening an issue
on github. WDYT?

> 
> 
> ## The only actionable feedback that I have that is specific to this commit is:
> 
> In the past, we have introduced new (non-test) Landlock functionality in a
> single commit -- that way, we have no "loose ends" in the code between these two
> commits, and that simplifies it for people who want to patch your feature onto
> other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
> into a single commit.)  WDYT?

Yeah, this two should be merged and tests commits as well. I just wanted
to do this in one of the latest patch versions to simplify code review.

> 
> —Günther