| Message ID | e0486aaa7f6a1ecdd12d0231d0b3ed560158a0f9.1533307527.git.g.nault@alphalink.fr |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl() | expand |
From: Guillaume Nault <g.nault@alphalink.fr> Date: Fri, 3 Aug 2018 17:00:11 +0200 > If 'session' is not NULL and is not a PPP pseudo-wire, then we fail to > drop the reference taken by l2tp_session_get(). > > Fixes: ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr> > --- > Sorry for the stupid mistake. I guess I got blinded by the apparent > simplicity of the bug when I wrote the original patch. Applied, thanks. I'm pretty sure I backported the commit this fixes, so I'm queueing this up for -stable as well.
On Fri, Aug 03, 2018 at 12:42:22PM -0700, David Miller wrote: > From: Guillaume Nault <g.nault@alphalink.fr> > Date: Fri, 3 Aug 2018 17:00:11 +0200 > > > If 'session' is not NULL and is not a PPP pseudo-wire, then we fail to > > drop the reference taken by l2tp_session_get(). > > > > Fixes: ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") > > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr> > > --- > > Sorry for the stupid mistake. I guess I got blinded by the apparent > > simplicity of the bug when I wrote the original patch. > > Applied, thanks. > > I'm pretty sure I backported the commit this fixes, so I'm queueing > this up for -stable as well. > Well, I think it wasn't. I didn't receive any notification from the stable team about it and I don't see it in Greg's stable queue nor in any -stable tree. Also, we'd have to queue 90904ff5f958 ("l2tp: fix pseudo-wire type for sessions created by pppol2tp_connect()") first, which is necessary for properly identifying PPP sessions. To recapitulate, three patches are needed to fix the original bug: * 90904ff5f958 ("l2tp: fix pseudo-wire type for sessions created by pppol2tp_connect()"): allows later patches to check if a session is PPP. * ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()"): refuses calling pppol2tp_session_ioctl() on non-PPP sessions. This fixes an invalid pointer dereference when the session is Ethernet. Unfortunately it fails to drop the reference it takes on the session. * f664e37dcc52 ("l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl()"): fixes the memory leak introduced by the previous patch.
On Sun, Aug 05, 2018 at 01:24:13PM +0200, Guillaume Nault wrote: > On Fri, Aug 03, 2018 at 12:42:22PM -0700, David Miller wrote: > > From: Guillaume Nault <g.nault@alphalink.fr> > > Date: Fri, 3 Aug 2018 17:00:11 +0200 > > > > > If 'session' is not NULL and is not a PPP pseudo-wire, then we fail to > > > drop the reference taken by l2tp_session_get(). > > > > > > Fixes: ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") > > > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr> > > > --- > > > Sorry for the stupid mistake. I guess I got blinded by the apparent > > > simplicity of the bug when I wrote the original patch. > > > > Applied, thanks. > > > > I'm pretty sure I backported the commit this fixes, so I'm queueing > > this up for -stable as well. > > > Well, I think it wasn't. I didn't receive any notification from the > stable team about it and I don't see it in Greg's stable queue nor > in any -stable tree. > > Also, we'd have to queue 90904ff5f958 ("l2tp: fix pseudo-wire type for > sessions created by pppol2tp_connect()") first, which is necessary for > properly identifying PPP sessions. > > To recapitulate, three patches are needed to fix the original bug: > > * 90904ff5f958 ("l2tp: fix pseudo-wire type for sessions created by > pppol2tp_connect()"): allows later patches to check if a session is > PPP. > > * ecd012e45ab5 ("l2tp: filter out non-PPP sessions in > pppol2tp_tunnel_ioctl()"): refuses calling pppol2tp_session_ioctl() > on non-PPP sessions. This fixes an invalid pointer dereference when > the session is Ethernet. Unfortunately it fails to drop the > reference it takes on the session. > > * f664e37dcc52 ("l2tp: fix missing refcount drop in > pppol2tp_tunnel_ioctl()"): fixes the memory leak introduced by the > previous patch. > Hi Dave, As far as I can see, f664e37dcc52 ("l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl()") is still in your -stable queue, but the two patches it depends on haven't made their way to -stable. I'd suggest to either drop this patch from your -stable queue, or to also queue up ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") and f664e37dcc52 ("l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl()").
From: Guillaume Nault <g.nault@alphalink.fr> Date: Fri, 10 Aug 2018 19:58:38 +0200 > As far as I can see, f664e37dcc52 ("l2tp: fix missing refcount drop in > pppol2tp_tunnel_ioctl()") is still in your -stable queue, but the two > patches it depends on haven't made their way to -stable. I'd suggest to > either drop this patch from your -stable queue, or to also queue up > ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") > and > f664e37dcc52 ("l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl()"). Thanks Guillaume, I'll sort this out the next time I work on stable submissions.
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index e398797878a9..cf6cca260e7b 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -1201,13 +1201,18 @@ static int pppol2tp_tunnel_ioctl(struct l2tp_tunnel *tunnel, l2tp_session_get(sock_net(sk), tunnel, stats.session_id); - if (session && session->pwtype == L2TP_PWTYPE_PPP) { - err = pppol2tp_session_ioctl(session, cmd, - arg); + if (!session) { + err = -EBADR; + break; + } + if (session->pwtype != L2TP_PWTYPE_PPP) { l2tp_session_dec_refcount(session); - } else { err = -EBADR; + break; } + + err = pppol2tp_session_ioctl(session, cmd, arg); + l2tp_session_dec_refcount(session); break; } #ifdef CONFIG_XFRM
If 'session' is not NULL and is not a PPP pseudo-wire, then we fail to drop the reference taken by l2tp_session_get(). Fixes: ecd012e45ab5 ("l2tp: filter out non-PPP sessions in pppol2tp_tunnel_ioctl()") Signed-off-by: Guillaume Nault <g.nault@alphalink.fr> --- Sorry for the stupid mistake. I guess I got blinded by the apparent simplicity of the bug when I wrote the original patch. net/l2tp/l2tp_ppp.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)