netdev - Re: [PATCH v4 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] [day] [month] [year] [list]

Date:   Thu, 16 Feb 2023 16:58:02 +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 v4 1/2] net/handshake: Create a NETLINK service for
 handling handshake requests


> On Feb 16, 2023, at 10:15 AM, Chuck Lever III <chuck.lever@...cle.com> wrote:
> 
>> 
>> On Feb 16, 2023, at 8:12 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> 
>> On Wed, 2023-02-15 at 14:23 -0500, Chuck Lever wrote:
>> 
>>> +static void __net_exit handshake_net_exit(struct net *net)
>>> +{
>>> +	struct handshake_req *req;
>>> +	LIST_HEAD(requests);
>>> +
>>> +	/*
>>> +	 * XXX: This drains the net's pending list, but does
>>> +	 *	nothing about requests that have been accepted
>>> +	 *	and are in progress.
>>> +	 */
>>> +	spin_lock(&net->hs_lock);
>>> +	list_splice_init(&requests, &net->hs_requests);
>>> +	spin_unlock(&net->hs_lock);
>> 
>> If I read correctly accepted, uncompleted reqs are leaked.
> 
> Yes, that's exactly right.
> 
> 
>> I think that
>> could be prevented installing a custom sk_destructor in sock->sk
>> tacking care of freeing the sk->sk_handshake_req. The existing/old
>> sk_destructor - if any - could be stored in an additional
>> sk_handshake_req field and tail-called by the req's one.
> 
> I've been looking for a way to modify socket close behavior
> for these sockets, and this sounds like it's in the
> neighborhood. I'll have a look.
> 
> So one thing we might do is have CMD_DONE act just as a way
> to report handshake results, and have the handshake daemon
> close the fd to signal it's finished with it. sk_destructor
> would then fire handshake_complete and free the
> handshake_req.
> 
> Might make things a little more robust?

->sk_destruct is the wrong place to hook, since we need the
socket itself to stay around after the handshake completes.
Better would be hooking the close of the user space file
descriptor.

Yes, we could use ->sk_destruct to tear down the handshake_req,
if we don't mind it sticking around until the socket is
finally closed.


--
Chuck Lever