On 06/18/2013 04:22 PM, Neil Horman wrote:
I like this idea, but I think I'm maybe missing something from it - we reference
the socket in both the receive and send paths (sctp_unpack_cookie, is
specifically called from the rx path, which makes use of sp->hmac). a socket
destructor can be called from __sk_free when sk_wmem_alloc reaches zero, but we
use sk_refcnt in the rx path to prevent premature socket cleanup. If we drain
our send queeue while wer'e still processing rx messages, what prevents us from
freeing the socket in the tx path, via sk_free while we're still using the
socket in the rx path. Note I don't think this patch is wrong per-se, but it
seems to me there is more work to do to properly interlock the use of sk_refcnt
and sk_wmem_alloc here (unless I'm just missing something obvious, which is
entirely possible, I've been in the sun alot lately :) ).
Hm, __sk_free() calls sk_prot_free() which frees our socket structure and in
sctp_wfree() we do a sctp_association_put(asoc) after sock_wfree(skb).
So no matter if having this patch or not, couldn't this use-after-free like
scenario already happen with the current code?
F.e. through a given call graph like that:
sctp_wfree(skb):
1) sock_wfree(skb)
-> __sk_free()
-> sk_prot_free(.., sk)
-> kmem_cache_free(.., sk) or kfree(sk)
2) __sctp_write_space(asoc)
3) sctp_association_put(asoc)
-> sctp_association_destroy(asoc)
-> sctp_endpoint_put(asoc->ep)
-> sctp_endpoint_destroy(ep)
-> crypto_free_hash(sctp_sk(ep->base.sk)->hmac)
(etc, all unconditionally accessed while sk is
already dead/freed)
Then, this might need a fix in general. :-)
Assuming you would reduce the buffer space via setsockopt(.., SO_SNDBUF, ..),
you might end up with a minimum buffer space of SOCK_MIN_SNDBUF [*] and a call to
sk->sk_write_space(sk), which is sctp_write_space() and calls __sctp_write_space()
on all asocs belonging to the socket, but it seems not to alter the current
sk->sk_wmem_alloc I think, but rather sk->sk_sndbuf.
[*] Btw, shouldn't this rather be (2048 + sizeof(struct sk_buff)) or
SKB_TRUESIZE(2048), at least like in SOCK_MIN_RCVBUF since we operate
on skb->truesize as well?
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
