netdev - Re: [PATCH v3 1/2] net/handshake: Create a NETLINK service for handling handshake requests

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: <68DCB255-772E-4F48-BC9B-AE2F50392402@oracle.com>
Date:   Fri, 10 Feb 2023 14:31:25 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Paolo Abeni <pabeni@...hat.com>
CC:     Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        "hare@...e.com" <hare@...e.com>,
        David Howells <dhowells@...hat.com>,
        Benjamin Coddington <bcodding@...hat.com>,
        Olga Kornievskaia <kolga@...app.com>,
        "jmeneghi@...hat.com" <jmeneghi@...hat.com>
Subject: Re: [PATCH v3 1/2] net/handshake: Create a NETLINK service for
 handling handshake requests



> On Feb 10, 2023, at 6:41 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> 
> On Thu, 2023-02-09 at 16:34 +0000, Chuck Lever III wrote:
>> 
>>> On Feb 9, 2023, at 11:02 AM, Paolo Abeni <pabeni@...hat.com> wrote:
>>> 
>>> On Thu, 2023-02-09 at 15:43 +0000, Chuck Lever III wrote:
>>>>> On Feb 9, 2023, at 1:00 AM, Jakub Kicinski <kuba@...nel.org> wrote:
>>>>> 
>>>>> On Tue, 07 Feb 2023 16:41:13 -0500 Chuck Lever wrote:
>>>>>> diff --git a/tools/include/uapi/linux/netlink.h
>>>>>> b/tools/include/uapi/linux/netlink.h
>>>>>> index 0a4d73317759..a269d356f358 100644
>>>>>> --- a/tools/include/uapi/linux/netlink.h
>>>>>> +++ b/tools/include/uapi/linux/netlink.h
>>>>>> @@ -29,6 +29,7 @@
>>>>>> #define NETLINK_RDMA		20
>>>>>> #define NETLINK_CRYPTO		21	/* Crypto layer */
>>>>>> #define NETLINK_SMC		22	/* SMC monitoring */
>>>>>> +#define NETLINK_HANDSHAKE	23	/* transport layer sec
>>>>>> handshake requests */
>>>>> 
>>>>> The extra indirection of genetlink introduces some complications?
>>>> 
>>>> I don't think it does, necessarily. But neither does it seem
>>>> to add any value (for this use case). <shrug>
>>> 
>>> To me it introduces a good separation between the handshake mechanism
>>> itself and the current subject (sock).
>>> 
>>> IIRC the previous version allowed the user-space to create a socket of
>>> the HANDSHAKE family which in turn accept()ed tcp sockets. That kind of
>>> construct - assuming I interpreted it correctly - did not sound right
>>> to me.
>>> 
>>> Back to these patches, they looks sane to me, even if the whole
>>> architecture is a bit hard to follow, given the non trivial cross
>>> references between the patches - I can likely have missed some relevant
>>> point.
>> 
>> One of the original goals was to support other security protocols
>> besides TLS v1.3, which is why the code is split between two
>> patches. I know that is cumbersome for some review workflows.
>> 
>> Now is a good time to simplify, if we see a sensible opportunity
>> to do so.
> 
> I think that adding a 'hi_free'/'hi_release' op inside the
> handshake_info struct - and moving the handshake info deallocation
> inside the 'core' could possibly simplify a bit the architecture.

I'm concerned about lifetime issues for handshake_info. I was
thinking maybe these objects need to be reference-counted as
well. I'll experiment with adding a destructor method too.


> Since it looks like there is a reasonable agreement on this path
> (@Dave, @Eric, @Jakub: please educate me otherwise!), and no
> clear/immediate show stoppers, I suggested start hammering some
> documentation with an high level overview that will help also
> understanding/reviewing the code.

In previous generations of this series, there was an addition
to Documentation/ that explained how kernel TLS consumers use
the tls_ handshake API. I can add that back now that things
are settling down.

But maybe you are thinking of some other topics. I'm happy to
write down whatever is needed, but I'd like suggestions about
what particular areas would be most helpful.


>>> I'm wondering if this approach scales well enough with the number of
>>> concurrent handshakes: the single list looks like a potential bottle-
>>> neck.
>> 
>> It's not clear how much scaling is needed. I don't have a strong
>> sense of how frequently a busy storage server will need a handshake,
>> for instance, but it seems like it would be relatively less frequent
>> than, say, I/O. Network storage connections are typically long-lived,
>> unlike http.
>> 
>> In terms of scalability, I am a little more concerned about the
>> handshake_mutex. Maybe that isn't needed since the pending list is
>> spinlock protected?
> 
> Good point. Indeed it looks like that is not needed.

I will remove the handshake_mutex in v4.


>> All that said, the single pending list can be replaced easily. It
>> would be straightforward to move it into struct net, for example.
> 
> In the end I don't see a operations needing a full list traversal.
> handshake_nl_msg_accept walk that, but it stops at netns/proto matching
> which should be ~always /~very soon in the typical use-case. And as you
> said it should be easy to avoid even that.
> 
> I think it could be useful limiting the number of pending handshake to
> some maximum, to avoid problems in pathological/malicious scenarios.

Defending against DoS is sensible. Maybe having a per-net
maximum of 5 or 10 pending handshakes? handshake_request() can
return an error code if a handshake is requested while we're
over that maximum.


--
Chuck Lever