netdev - Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation

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: <7940eadb-a01c-172f-5c57-e372993f4bd9@oracle.com>
Date:   Thu, 1 Mar 2018 20:19:57 +0800
From:   Ka-Cheong Poon <ka-cheong.poon@...cle.com>
To:     Sowmini Varadhan <sowmini05@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, santosh.shilimkar@...cle.com,
        David Miller <davem@...emloft.net>, rds-devel@....oracle.com,
        Sowmini Varadhan <sowmini.varadhan@...cle.com>
Subject: Re: [PATCH net] rds: Incorrect reference counting in TCP socket
 creation

On 03/01/2018 07:54 PM, Sowmini Varadhan wrote:
> On Wed, Feb 28, 2018 at 11:44 PM, Ka-Cheong Poon
> <ka-cheong.poon@...cle.com> wrote:
>> Commit 0933a578cd55 ("rds: tcp: use sock_create_lite() to create the
>> accept socket") has a reference counting issue in TCP socket creation
>> when accepting a new connection.  The code uses sock_create_lite() to
>> create a kernel socket.  But it does not do __module_get() on the
>> socket owner.  When the connection is shutdown and sock_release() is
>> called to free the socket, the owner's reference count is decremented
>> and becomes incorrect.  Note that this bug only shows up when the socket
>> owner is configured as a kernel module.
> 
>>
>> -       new_sock->type = sock->type;
>> -       new_sock->ops = sock->ops;
>>          ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true);
>>          if (ret < 0)
>>                  goto out;
>>
>> +       new_sock->ops = sock->ops;
> 
> How is this delta relevant to the commit comment? Seems unrelated?


Note that sock_release() checks if sock->ops is set before
decrementing the refcnt.  By moving the ops assignment after
the ops->accept() call, we save increasing the refcnt in
case the ops->accept() fails.  Otherwise, the __module_get()
needs to be moved before ops->accept() to handle this failure
case.


-- 
K. Poon
ka-cheong.poon@...cle.com