netdev - Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto

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: <20190222025157.GC2217@ZenIV.linux.org.uk>
Date:   Fri, 22 Feb 2019 02:51:57 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Mao Wenan <maowenan@...wei.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Lorenzo Colitti <lorenzo@...gle.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH net] net: socket: set sock->sk to NULL after calling
 proto_ops::release()

On Thu, Feb 21, 2019 at 02:13:56PM -0800, Eric Biggers wrote:

> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().

	Is there any case when we would want sock->sk non-NULL when
sock->sk->sk_socket is NULL?

	IOW, why not make sock_orphan() take care of that, at the same
time we dissociate sock from socket?  After all, on the other end
of things we have both sock_graft() and sock_init_data() set both
sock->sk and sk->sk_socket...

	Looking at the assignments to sock->sk, I see
* atalk_release(), bcm_release(), raw_release(), pfkey_release(),
pppol2tp_release(), packet_release(), smc_release(), xsk_release() -
directly after sock_orphan()
* rxrpc_release() - directly *before* sock_orphan()

* ax25_release() - somewhat after sock_orphan(); no idea if anything
done in between needs it still set.  Doesn't looks like there could
be - readers of sock->sk in there are all in methods that can't
overlap with ->release().
* kcm_release() - similar.
* rds_release() - similar.
* tipc_release() - similar.
* unix_accept() - similar, and there I'm fairly sure it could be
done right after sock_orphan()
* netrom_release() - similar.  BTW, why the hell does nr_accept()
check for NULL sock->sk, while other methods (callable for exact
same sockets) assume that it's non-NULL?  AFAICS, the check in
nr_accept() is pointless - nr_create() can leave sock->sk NULL
only if it fails, in which case it's going to be immediately
hit by ->release() and freed by iput() in sock_release().
* rose_release() - similar, complete with ->accept() oddity.

* caif_release() - done *before* sock_orphan(), with other bit of
the latter duplicated just prior.  NFI why; it messes with
debugfs, so there's a whole kettle of copulating slugs involved ;-/

* ieee802154_sock_release(), inet_release(), pn_socket_release() -
done before calliing into protocol's ->close(), which ends up
calling sock_orphan(), either directly or via sk_common_release().
I suspect that we would be find with zeroing sock->sk delayed
until sock_orphan() in all of those, but that needs more RTFS
than I want to go into at the moment.

* netlink_release() - somewhat after sock_orphan(); not sure,
calls of ->netlink_unbind() done inbetween might want it still
set.

* qrtr_release() - lacks sock_orphan(), might be broken

* vsock_releae() - NFI.

* vcc_create() - clears sock->sk in the very beginning; AFAICS
that's pointless.