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: Mon, 27 May 2024 10:48:58 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.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

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?
  * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?
  * Can we refactor out commonalities (probably not worth it right now though)?

## 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?

—Günther