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

Message-ID: <30125584-BB17-4727-B202-3D661625845D@oracle.com>
Date:   Wed, 5 Apr 2023 14:25:37 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Chuck Lever <cel@...nel.org>, 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>\"@mx0b-00069f01.pphosted.com" 
        <"kernel-tls-handshake@...ts.linux.dev\" <kernel-tls-handshake@...ts.linux.dev>, John Haxby <john.haxby@...cle.com>"@mx0b-00069f01.pphosted.com>
Subject: Re: [PATCH v8 1/4] net/handshake: Create a NETLINK service for
 handling handshake requests



> On Apr 4, 2023, at 9:32 PM, Jakub Kicinski <kuba@...nel.org> wrote:
> 
> On Wed, 5 Apr 2023 00:44:11 +0000 Chuck Lever III wrote:
>>> On Mon, 03 Apr 2023 14:46:02 -0400 Chuck Lever wrote:  
>>>> +int handshake_nl_done_doit(struct sk_buff *skb, struct genl_info *info)
>>>> +{
>>>> + struct net *net = sock_net(skb->sk);
>>>> + struct socket *sock = NULL;
>>>> + struct handshake_req *req;
>>>> + int fd, status, err;
>>>> +
>>>> + if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_DONE_SOCKFD))
>>>> + return -EINVAL;
>>>> + fd = nla_get_u32(info->attrs[HANDSHAKE_A_DONE_SOCKFD]);
>>>> +
>>>> + err = 0;
>>>> + sock = sockfd_lookup(fd, &err);
>>>> + if (err) {
>>>> + err = -EBADF;
>>>> + goto out_status;
>>>> + }
>>>> +
>>>> + req = handshake_req_hash_lookup(sock->sk);
>>>> + if (!req) {  
>>> 
>>> fput() missing on this path?  
>> 
>> fput of ... sock->file? DONE shouldn't do that if
>> it can't find the sock's matching handshake_req.
> 
> Hm, sounds odd. sockfd_lookup() returns a sock with incremented
> reference count, so if user space passes a random fd unrelated 
> to any real request - we'll bump the refcount of that fd and
> return  an error. So the related file is going to get leaked.
> What did I miss? 🤔️

Makes sense, got it. I'll add something here.


>>>> + err = -EBUSY;
>>>> + goto out_status;
>>>> + }
>>>> +
>>>> + trace_handshake_cmd_done(net, req, sock->sk, fd);
>>>> +
>>>> + status = -EIO;
>>>> + if (info->attrs[HANDSHAKE_A_DONE_STATUS])
>>>> + status = nla_get_u32(info->attrs[HANDSHAKE_A_DONE_STATUS]);
>>>> +
>>>> + handshake_complete(req, status, info);
>>>> + fput(sock->file);
>>>> + return 0;
>>>> +
>>>> +out_status:
>>>> + trace_handshake_cmd_done_err(net, req, sock->sk, err);
>>>> + return err;
>>>> +}  
>>> 
>>>> + /*
>>>> + * Arbitrary limit to prevent handshakes that do not make
>>>> + * progress from clogging up the system.
>>>> + */
>>>> + si_meminfo(&si);
>>>> + tmp = si.totalram / (25 * si.mem_unit);
>>>> + hn->hn_pending_max = clamp(tmp, 3UL, 25UL);  
>>> 
>>> No idea what this does (what's mem_unit?), we'll have to trust you :)  
>> 
>> Paolo requested that we link the pending_max limit to
>> the memory size of the system. I thought folks would
>> be familiar with the si_meminfo() kernel API.
>> 
>> What does it need? A better comment? A different approach?
> 
> I think comment would be good. Are you clamping to 1/25th of all
> available pages? The request take up about a page?

As the sign sez, it's an arbitrary cap. I just picked a
formula that gave about 10 pending entries on my tiny
test system.

As always, suggestions are welcome.

--
Chuck Lever