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]

Date:   Wed, 5 Apr 2023 08:32:23 +0200
From:   Hannes Reinecke <hare@...e.de>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Chuck Lever III <chuck.lever@...cle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        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/5/23 02:00, Jakub Kicinski wrote:
> On Tue, 4 Apr 2023 17:44:19 +0200 Hannes Reinecke wrote:
>>> 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.
> 
> Ah, I'm probably the weakest of the netdev maintainers when it comes
> to the socket layer :)
> 
> I thought sock->sk is only cleared if the "user" of the socket closes
> it. But yes, both sock->sk == NULL and sk->sk_socket == NULL are
> entirely possible, and the networking stack usually operates on
> struct sock. Why exactly those two are separate beings is one of
> the mysteries of Linux networking which causes guaranteed confusion
> to all the newcomers. I wish I knew the details so I could at least
> document it :S

Bummer. I had high hopes on you being able to shed some light on this.

So, Chuck: maybe we should be looking at switching over to 'struct sock' 
for the internal stuff. If we don't have to do a 'fput()' somewhere we 
should be good...

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