From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CF46CC433E1 for ; Sat, 13 Jun 2020 15:59:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B06832074D for ; Sat, 13 Jun 2020 15:59:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726765AbgFMP7L (ORCPT ); Sat, 13 Jun 2020 11:59:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35068 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726609AbgFMP6W (ORCPT ); Sat, 13 Jun 2020 11:58:22 -0400 Received: from smtp.tuxdriver.com (tunnel92311-pt.tunnel.tserv13.ash1.ipv6.he.net [IPv6:2001:470:7:9c9::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 464A0C08C5C1; Sat, 13 Jun 2020 08:58:22 -0700 (PDT) Received: from 2606-a000-111b-4634-0000-0000-0000-1bf2.inf6.spectrum.com ([2606:a000:111b:4634::1bf2] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1jk8XY-0002Cu-MZ; Sat, 13 Jun 2020 11:57:55 -0400 Date: Sat, 13 Jun 2020 11:57:51 -0400 From: Neil Horman To: Xiyu Yang Cc: Vlad Yasevich , Marcelo Ricardo Leitner , "David S. Miller" , Jakub Kicinski , linux-sctp@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, yuanxzhang@fudan.edu.cn, kjlu@umn.edu, Xin Tan Subject: Re: [PATCH] sctp: Fix sk_buff leak when receiving a datagram Message-ID: <20200613155751.GA161691@hmswarspite.think-freely.org> References: <1592051965-94731-1-git-send-email-xiyuyang19@fudan.edu.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1592051965-94731-1-git-send-email-xiyuyang19@fudan.edu.cn> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Jun 13, 2020 at 08:39:25PM +0800, Xiyu Yang wrote: > 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 > Signed-off-by: Xin Tan > --- > 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 > >