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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 30 May 2024 15:20:21 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.com>
CC: <mic@...ikod.net>, <willemdebruijn.kernel@...il.com>,
	<gnoack3000@...il.com>, <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 v2 02/12] landlock: Add hook on socket creation



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@...wei-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