On 03/04/2014 10:35 AM, Daniel Borkmann wrote:
> While working on ec0223ec48a9 ("net: sctp: fix sctp_sf_do_5_1D_ce to
> verify if we/peer is AUTH capable"), we noticed that there's a skb
> memory leakage in the error path.
>
> Running the same reproducer as in ec0223ec48a9 and by unconditionally
> jumping to the error label (to simulate an error condition) in
> sctp_sf_do_5_1D_ce() receive path lets kmemleak detector bark about
> the unfreed chunk->auth_chunk skb clone:
>
> Unreferenced object 0xffff8800b8f3a000 (size 256):
> comm "softirq", pid 0, jiffies 4294769856 (age 110.757s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 89 ab 75 5e d4 01 58 13 00 00 00 00 00 00 00 00 ..u^..X.........
> backtrace:
> [<ffffffff816660be>] kmemleak_alloc+0x4e/0xb0
> [<ffffffff8119f328>] kmem_cache_alloc+0xc8/0x210
> [<ffffffff81566929>] skb_clone+0x49/0xb0
> [<ffffffffa0467459>] sctp_endpoint_bh_rcv+0x1d9/0x230 [sctp]
> [<ffffffffa046fdbc>] sctp_inq_push+0x4c/0x70 [sctp]
> [<ffffffffa047e8de>] sctp_rcv+0x82e/0x9a0 [sctp]
> [<ffffffff815abd38>] ip_local_deliver_finish+0xa8/0x210
> [<ffffffff815a64af>] nf_reinject+0xbf/0x180
> [<ffffffffa04b4762>] nfqnl_recv_verdict+0x1d2/0x2b0 [nfnetlink_queue]
> [<ffffffffa04aa40b>] nfnetlink_rcv_msg+0x14b/0x250 [nfnetlink]
> [<ffffffff815a3269>] netlink_rcv_skb+0xa9/0xc0
> [<ffffffffa04aa7cf>] nfnetlink_rcv+0x23f/0x408 [nfnetlink]
> [<ffffffff815a2bd8>] netlink_unicast+0x168/0x250
> [<ffffffff815a2fa1>] netlink_sendmsg+0x2e1/0x3f0
> [<ffffffff8155cc6b>] sock_sendmsg+0x8b/0xc0
> [<ffffffff8155d449>] ___sys_sendmsg+0x369/0x380
>
> What happens is that commit bbd0d59809f9 clones the skb containing
> the AUTH chunk in sctp_endpoint_bh_rcv() when having the edge case
> that an endpoint requires COOKIE-ECHO chunks to be authenticated:
>
> ---------- INIT[RANDOM; CHUNKS; HMAC-ALGO] ---------->
> <------- INIT-ACK[RANDOM; CHUNKS; HMAC-ALGO] ---------
> ------------------ AUTH; COOKIE-ECHO ---------------->
> <-------------------- COOKIE-ACK ---------------------
>
> When we enter sctp_sf_do_5_1D_ce() and before we actually get to
> the point where we process (and subsequently free) a non-NULL
> chunk->auth_chunk, we could hit the "goto nomem_init" path from
> an error condition and thus leave the cloned skb around w/o
> freeing it.
>
> The fix is to centrally free such clones in sctp_chunk_destroy()
> handler that is invoked from sctp_chunk_free() after all refs have
> dropped; and also move both kfree_skb(chunk->auth_chunk) there,
> so that chunk->auth_chunk is either NULL (since sctp_chunkify()
> allocs new chunks through kmem_cache_zalloc()) or non-NULL with
> a valid skb pointer. chunk->skb and chunk->auth_chunk are the
> only skbs in the sctp_chunk structure that need to be handeled.
>
> While at it, we should use consume_skb() for both. It is the same
> as dev_kfree_skb() but more appropriately named as we are not
> a device but a protocol. Also, this effectively replaces the
> kfree_skb() from both invocations into consume_skb(). Functions
> are the same only that kfree_skb() assumes that the frame was
> being dropped after a failure (e.g. for tools like drop monitor),
> usage of consume_skb() seems more appropriate in function
> sctp_chunk_destroy() though.
>
> Fixes: bbd0d59809f9 ("[SCTP]: Implement the receive and verification of AUTH chunk")
> Signed-off-by: Daniel Borkmann <dborkman@xxxxxxxxxx>
> Cc: Vlad Yasevich <yasevich@xxxxxxxxx>
> Cc: Neil Horman <nhorman@xxxxxxxxxxxxx>
Acked-by: Vlad Yasevich <vyasevich@xxxxxxxxx>
-vlad
> ---
> net/sctp/sm_make_chunk.c | 4 ++--
> net/sctp/sm_statefuns.c | 5 -----
> 2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 632090b..3a1767e 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1421,8 +1421,8 @@ static void sctp_chunk_destroy(struct sctp_chunk *chunk)
> BUG_ON(!list_empty(&chunk->list));
> list_del_init(&chunk->transmitted_list);
>
> - /* Free the chunk skb data and the SCTP_chunk stub itself. */
> - dev_kfree_skb(chunk->skb);
> + consume_skb(chunk->skb);
> + consume_skb(chunk->auth_chunk);
>
> SCTP_DBG_OBJCNT_DEC(chunk);
> kmem_cache_free(sctp_chunk_cachep, chunk);
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index ae65b6b..01e0024 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -760,7 +760,6 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
>
> /* Make sure that we and the peer are AUTH capable */
> if (!net->sctp.auth_enable || !new_asoc->peer.auth_capable) {
> - kfree_skb(chunk->auth_chunk);
> sctp_association_free(new_asoc);
> return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> }
> @@ -775,10 +774,6 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
> auth.transport = chunk->transport;
>
> ret = sctp_sf_authenticate(net, ep, new_asoc, type, &auth);
> -
> - /* We can now safely free the auth_chunk clone */
> - kfree_skb(chunk->auth_chunk);
> -
> if (ret != SCTP_IERROR_NO_ERROR) {
> sctp_association_free(new_asoc);
> return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>
--
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
