netdev - Re: [PATCH v8 1/4] 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: <4dde688e-21db-6cc6-080e-c451eac2a9ca@suse.de>
Date:   Tue, 4 Apr 2023 17:44:19 +0200
From:   Hannes Reinecke <hare@...e.de>
To:     Chuck Lever III <chuck.lever@...cle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     Boris Pismenny <borisp@...dia.com>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        "kernel-tls-handshake@...ts.linux.dev" 
        <kernel-tls-handshake@...ts.linux.dev>,
        John Haxby <john.haxby@...cle.com>
Subject: Re: [PATCH v8 1/4] net/handshake: Create a NETLINK service for
 handling handshake requests

On 4/4/23 17:36, Chuck Lever III wrote:
> 
> 
>> On Apr 3, 2023, at 2:46 PM, Chuck Lever <cel@...nel.org> wrote:
>>
[ .. ]
>> +/**
>> + * handshake_req_cancel - Cancel an in-progress handshake
>> + * @sock: socket on which there is an ongoing handshake
>> + *
>> + * Request cancellation races with request completion. To determine
>> + * who won, callers examine the return value from this function.
>> + *
>> + * Return values:
>> + *   %true - Uncompleted handshake request was canceled or not found
>> + *   %false - Handshake request already completed
>> + */
>> +bool handshake_req_cancel(struct socket *sock)
>> +{
>> + struct handshake_req *req;
>> + struct handshake_net *hn;
>> + struct sock *sk;
>> + struct net *net;
>> +
>> + sk = sock->sk;
>> + net = sock_net(sk);
> 
> We're still seeing NULL pointer dereferences here.
> Typically this happens after the remote closes the
> connection early.
> 
> I guess I cannot rely on sock_hold(sk); from preventing
> someone from doing a "sock->sk = NULL;"
> 
> Would it make more sense for req_submit and req_cancel to
> operate on "struct sock *" rather than "struct socket *" ?
> 
Stumbled across that one, too; that's why my initial submission
was sprinkled with 'if (!sock->sk)' statements.
So I think it's a good idea.

But waiting for Jakub to enlighten us.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@...e.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman