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] [day] [month] [year] [list]

Message-ID: <fd3760b7-7e66-e426-22fe-24170cf43f33@huawei-partners.com>
Date: Sat, 5 Oct 2024 21:32:01 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack3000@...il.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 10/5/2024 9:22 PM, Günther Noack wrote:
> 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.

Yes, these 2 release_lock()s can really raise questions when reading.

> 
> 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 :)).

Ok

> 
>>>> +
>>>> +release_nocheck:
>>>> +	release_sock(sk);
>>>> +	return err;
>>>> +}
> 
> –Günther