netdev - Re: [RFC PATCH v2 2/9] landlock: Support TCP listen access-control

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]

Message-ID: <20241005.e820f4fae74e@gnoack.org>
Date: Sat, 5 Oct 2024 20:22:54 +0200
From: Günther Noack <gnoack3000@...il.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: mic@...ikod.net, willemdebruijn.kernel@...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 2/9] landlock: Support TCP listen access-control

On Sat, Oct 05, 2024 at 08:53:55PM +0300, Mikhail Ivanov wrote:
> On 10/5/2024 7:56 PM, Günther Noack wrote:
> > On Wed, Aug 14, 2024 at 11:01:44AM +0800, Mikhail Ivanov wrote:
> > > +	port = htons(inet_sk(sk)->inet_num);
> > > +	release_sock(sk);
> > > +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
> > 
> > Nit: The last two lines could just be
> > 
> >    err = check_access_socket(...);
> > 
> > and then you would only need the release_sock(sk) call in one place.
> > (And maybe rename the goto label accordingly.)
> This split was done in order to not hold socket lock while doing some
> Landlock-specific logic. It might be identical in performance to
> your suggestion, but I thought that (1) security module should have as
> little impact on network stack as possible and (2) it is more
> clear that locking is performed only for a few socket state checks which
> are not related to the access control.
> 
> I'll add this explanation with a comment if you agree that everything is
> correct.


IMHO, when you grab a lock in this function, it is clear that you'd
unconditionally want to release it before you return from the
function, and that in C, the normal way to guarantee unconditional
cleanup work would be to apply the "single exit point" rule.

That being said, the scenario is simple enough here that it's not a
big issue in my eyes.  It was more of a minor nit about having more
than one place where the lock has to be released.  Either way is fine
(and also should not require excessive comments :)).

> > > +
> > > +release_nocheck:
> > > +	release_sock(sk);
> > > +	return err;
> > > +}

–Günther