Re: [PATCH] sctp: Fix sk_buff leak when receiving a datagram
From: Neil Horman <nhorman@tuxdriver.com>
Date: 2020-06-13 16:41:54
Also in:
linux-sctp, lkml
On Sat, Jun 13, 2020 at 08:39:25PM +0800, Xiyu Yang wrote:
quoted hunk ↗ jump to hunk
In sctp_skb_recv_datagram(), the function fetch a sk_buff object from the receiving queue to "skb" by calling skb_peek() or __skb_dequeue() and return its reference to the caller. However, when calling __skb_dequeue() successfully, the function forgets to hold a reference count of the "skb" object and directly return it, causing a potential memory leak in the caller function. Fix this issue by calling refcount_inc after __skb_dequeue() successfully executed. Signed-off-by: Xiyu Yang <redacted> Signed-off-by: Xin Tan <redacted> --- net/sctp/socket.c | 2 ++ 1 file changed, 2 insertions(+)diff --git a/net/sctp/socket.c b/net/sctp/socket.c index d57e1a002ffc..4c8f0b83efd0 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c@@ -8990,6 +8990,8 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, refcount_inc(&skb->users); } else { skb = __skb_dequeue(&sk->sk_receive_queue); + if (skb) + refcount_inc(&skb->users);
For completeness, you should probably use skb_get here, rather than refcount_inc directly. Also, I'm not entirely sure I see how a memory leak can happen here. we take an extra reference in the skb_peek clause of this code area because if we return an skb that continues to exist on the sk_receive_queue list, we legitimately have two users for the skb (the user who called sctp_skb_recv_datagram(...,MSG_PEEK), and the potential next caller who will actually dequeue the skb. In the else clause however, that condition doesn't exist. the user count for the skb should alreday be 1, if the caller is the only user of the skb), or more than 1, if 1 or more callers have gotten a reference to the message using MSG_PEEK. I don't think this code is needed, and in fact will actually cause memory leaks, because theres no subsequent skb_unref call to drop refcount that you are adding here. Neil
} if (skb) -- 2.7.4