From: Tom Parkin <tparkin@katalix.com>
To: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Cong Wang <cong.wang@bytedance.com>,
Randy Dunlap <rdunlap@infradead.org>,
Xin Xiong <xiongx18@fudan.edu.cn>,
"Gong, Sishuai" <sishuai@purdue.edu>,
Matthias Schiffer <mschiffer@universe-factory.net>,
Bhaskar Chowdhury <unixbhaskar@gmail.com>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
yuanxzhang@fudan.edu.cn, Xin Tan <tanxin.ctf@gmail.com>
Subject: Re: [PATCH] net/l2tp: Fix reference count leak in l2tp_udp_recv_core
Date: Thu, 9 Sep 2021 10:10:05 +0100 [thread overview]
Message-ID: <20210909091005.GB7098@katalix.com> (raw)
In-Reply-To: <20210909090156.GA7098@katalix.com>
[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]
On Thu, Sep 09, 2021 at 10:01:56 +0100, Tom Parkin wrote:
> On Thu, Sep 09, 2021 at 12:32:00 +0800, Xiyu Yang wrote:
> > The reference count leak issue may take place in an error handling
> > path. If both conditions of tunnel->version == L2TP_HDR_VER_3 and the
> > return value of l2tp_v3_ensure_opt_in_linear is nonzero, the function
> > would directly jump to label invalid, without decrementing the reference
> > count of the l2tp_session object session increased earlier by
> > l2tp_tunnel_get_session(). This may result in refcount leaks.
>
> I agree with your analysis. Thanks for catching this!
>
Also: I forgot to mention I think this fixes:
4522a70db7aa ("l2tp: fix reading optional fields of L2TPv3")
> >
> > Fix this issue by decrease the reference count before jumping to the
> > label invalid.
> >
> > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> > Signed-off-by: Xin Xiong <xiongx18@fudan.edu.cn>
> > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> > ---
> > net/l2tp/l2tp_core.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index 53486b162f01..93271a2632b8 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -869,8 +869,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> > }
> >
> > if (tunnel->version == L2TP_HDR_VER_3 &&
> > - l2tp_v3_ensure_opt_in_linear(session, skb, &ptr, &optr))
> > + l2tp_v3_ensure_opt_in_linear(session, skb, &ptr, &optr)) {
> > + l2tp_session_dec_refcount(session);
> > goto invalid;
> > + }
>
> The error paths in l2tp_udp_recv_core are a bit convoluted because of
> the check (!session || !session->recv_skb) which may or may not need
> to drop a session reference if triggered.
>
> I think it could be simplified since session->recv_skb is always set
> for all the current session types in the tree, but doing that is probably
> a little patch series on its own.
>
> >
> > l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
> > l2tp_session_dec_refcount(session);
> > --
> > 2.7.4
> >
>
> --
> Tom Parkin
> Katalix Systems Ltd
> https://katalix.com
> Catalysts for your Embedded Linux software development
--
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-09-09 9:10 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-09 4:32 [PATCH] net/l2tp: Fix reference count leak in l2tp_udp_recv_core Xiyu Yang
2021-09-09 9:01 ` Tom Parkin
2021-09-09 9:10 ` Tom Parkin [this message]
2021-09-09 10:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210909091005.GB7098@katalix.com \
--to=tparkin@katalix.com \
--cc=cong.wang@bytedance.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mschiffer@universe-factory.net \
--cc=netdev@vger.kernel.org \
--cc=rdunlap@infradead.org \
--cc=sishuai@purdue.edu \
--cc=tanxin.ctf@gmail.com \
--cc=unixbhaskar@gmail.com \
--cc=xiongx18@fudan.edu.cn \
--cc=xiyuyang19@fudan.edu.cn \
--cc=yuanxzhang@fudan.edu.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.