From dwmalone@maths.tcd.ie Mon Feb 7 07:15:42 2000 Return-Path: Received: from salmon.maths.tcd.ie (salmon.maths.tcd.ie [134.226.81.11]) by builder.freebsd.org (Postfix) with SMTP id 610263EF6; Mon, 7 Feb 2000 07:15:26 -0800 (PST) Received: from walton.maths.tcd.ie by salmon.maths.tcd.ie with SMTP id ; 7 Feb 2000 15:16:05 +0000 (GMT) Message-Id: <200002071516.aa82012@walton.maths.tcd.ie> Date: Mon, 7 Feb 2000 15:16:05 +0000 (GMT) From: dwmalone@maths.tcd.ie Sender: dwmalone@maths.tcd.ie Reply-To: dwmalone@maths.tcd.ie To: FreeBSD-gnats-submit@freebsd.org, ru@freebsd.org, ache@freebsd.org, bde@freebsd.org Subject: SLIOCSUNIT is broken and can cause panic. X-Send-Pr-Version: 3.2 >Number: 16564 >Category: kern >Synopsis: SLIOCSUNIT is broken and can cause panic. >Confidential: no >Severity: non-critical >Priority: medium >Responsible: ru >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Mon Feb 7 07:20:00 PST 2000 >Closed-Date: Wed Mar 1 05:39:35 PST 2000 >Last-Modified: Wed Mar 1 05:41:06 PST 2000 >Originator: David Malone >Release: FreeBSD 4.0-CURRENT i386 >Organization: School of Mathematics, Trinity College, Dublin 2, Ireland. >Environment: 4.0 as of the last few days, but I suspect the problem goes back a long way. >Description: The slip SLIOCSUNIT ioctl is supposed to allow you to decide what slip interface you are configuring and attaching to a tty, however it does some slightly strange things (see if_sl.c line 375): 1) Search for desired slip unit. 2) Swap the contents of original slip unit's softc and desired slip unit's softc. 3) Point the tty at the desired softc. The main problem is the sotfc for slip contains pointers which point to other bits of the softc (sc.sc_comp.last_cs, sc.sc_comp.tstate[n].cs_next and sc.sc_comp.rstate[n].cs_next), so copying the whole softc to a different location is incorrect. I'm not actually convinced that swapping the softc's is the correct action either, but I can't find any documentation for SLIOCSUNIT ioctl, so I can't be certain. Looking through /usr/src, calls to SLIOCSUNIT seem to be directly after switching to the tty to SLIPDISC, which means the swap is intended to leave the desired softc as if it had just been opened and leave the original softc in an unused state. >How-To-Repeat: Configure a kernel with two slip devices, then: slattach -a -c -h -S 1 -s 57600 /dev/ttyd0 ifconfig sl0 inet 10.0.1.1 10.0.1.4 netmask 255.255.255.0 telnet 10.0.1.4 You'll need something at the other end of the slip connection. Pings work fine 'cos they are not compressed - any tcp connection will result in a null pointer dereference at slcompress.c line 197. >Fix: Various options: 1) Teach SLIOCSUNIT how to swap the contents of sc.sc_comp, which seems a bit ugly. The code already swaps the sc.sc_if back again, but swapping sc.sc_comp would not be as straight forward, as the one that you want may not be initialised. 2) Make sc.sc_comp a pointer instead of an included structure. Seems straight forward, but a bit of a workaround rather than a fix. 3) Make SLIOCSUNIT do the equivelent of a slclose(original unit) and then a slopen(desired unit), so you are sure everything is correctly initialised. This is a relatively clean option but means you no longer swap the contents of the softc's. It would also remove some of the workaround code added for sc.sc_if. If someone can offer me guideance on which of these is the correct fix I can code and test it. >Release-Note: >Audit-Trail: From: Ruslan Ermilov To: dwmalone@maths.tcd.ie Cc: FreeBSD-gnats-submit@FreeBSD.ORG, ache@FreeBSD.ORG, bde@FreeBSD.ORG Subject: Re: kern/16564: SLIOCSUNIT is broken and can cause panic. Date: Thu, 17 Feb 2000 16:44:57 +0200 --2fHTh5uZTiUOsy+g Content-Type: text/plain; charset=us-ascii On Mon, Feb 07, 2000 at 03:16:05PM +0000, dwmalone@maths.tcd.ie wrote: > > >Number: 16564 > >Category: kern > >Synopsis: SLIOCSUNIT is broken and can cause panic. > >Originator: David Malone > >Release: FreeBSD 4.0-CURRENT i386 > > 4.0 as of the last few days, but I suspect the problem goes > back a long way. > Yup, the same panic in sl_compress_tcp() on -stable due to the NULL pointer dereference. > >Description: > > The slip SLIOCSUNIT ioctl is supposed to allow you to decide what > slip interface you are configuring and attaching to a tty, however > it does some slightly strange things (see if_sl.c line 375): > > 1) Search for desired slip unit. > 2) Swap the contents of original slip unit's softc and > desired slip unit's softc. > 3) Point the tty at the desired softc. > > The main problem is the sotfc for slip contains pointers which > point to other bits of the softc (sc.sc_comp.last_cs, > sc.sc_comp.tstate[n].cs_next and sc.sc_comp.rstate[n].cs_next), so > copying the whole softc to a different location is incorrect. > You are correct! > I'm not actually convinced that swapping the softc's is the correct > action either, but I can't find any documentation for SLIOCSUNIT > ioctl, so I can't be certain. Looking through /usr/src, calls to > SLIOCSUNIT seem to be directly after switching to the tty to > SLIPDISC, which means the swap is intended to leave the desired > softc as if it had just been opened and leave the original softc > in an unused state. > I don't like this as well, but do not have both quick and elegant solution, and we are too close to the release. > >How-To-Repeat: > > Configure a kernel with two slip devices, then: > > slattach -a -c -h -S 1 -s 57600 /dev/ttyd0 > ifconfig sl0 inet 10.0.1.1 10.0.1.4 netmask 255.255.255.0 > telnet 10.0.1.4 > > You'll need something at the other end of the slip connection. > Pings work fine 'cos they are not compressed - any tcp connection > will result in a null pointer dereference at slcompress.c line 197. > > >Fix: > > Various options: > > 1) Teach SLIOCSUNIT how to swap the contents of sc.sc_comp, > which seems a bit ugly. The code already swaps the sc.sc_if > back again, but swapping sc.sc_comp would not be as straight > forward, as the one that you want may not be initialised. > > 2) Make sc.sc_comp a pointer instead of an included structure. > Seems straight forward, but a bit of a workaround rather > than a fix. > > 3) Make SLIOCSUNIT do the equivelent of a slclose(original > unit) and then a slopen(desired unit), so you are sure > everything is correctly initialised. This is a relatively > clean option but means you no longer swap the contents of > the softc's. It would also remove some of the workaround > code added for sc.sc_if. > I like 3), but I'm not sure it is so simple. Could you please try the attached patch? -- Ruslan Ermilov Sysadmin and DBA of the ru@ucb.crimea.ua United Commercial Bank, ru@FreeBSD.org FreeBSD committer, +380.652.247.647 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age --2fHTh5uZTiUOsy+g Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=p Index: if_sl.c =================================================================== RCS file: /usr/FreeBSD-CVS/src/sys/net/if_sl.c,v retrieving revision 1.70.2.3 diff -u -p -r1.70.2.3 if_sl.c --- if_sl.c 1999/12/15 09:17:29 1.70.2.3 +++ if_sl.c 2000/02/17 14:23:09 @@ -405,6 +405,7 @@ sltioctl(tp, cmd, data, flag, p) clist_alloc_cblocks(&tp->t_outq, SLIP_HIWAT + 2 * sc->sc_if.if_mtu + 1, SLIP_HIWAT + 2 * sc->sc_if.if_mtu + 1); + sl_compress_init(&sc->sc_comp, -1); goto slfound; } } --2fHTh5uZTiUOsy+g-- From: David Malone To: Ruslan Ermilov Cc: FreeBSD-gnats-submit@FreeBSD.ORG, ache@FreeBSD.ORG, bde@FreeBSD.ORG Subject: Re: kern/16564: SLIOCSUNIT is broken and can cause panic. Date: Thu, 17 Feb 2000 20:17:41 +0000 > > 3) Make SLIOCSUNIT do the equivelent of a slclose(original > > unit) and then a slopen(desired unit), so you are sure > > everything is correctly initialised. This is a relatively > > clean option but means you no longer swap the contents of > > the softc's. It would also remove some of the workaround > > code added for sc.sc_if. > > > I like 3), but I'm not sure it is so simple. I think it would be (relatively) easy to factor the apropriate bits of slclose and slopen into internal functions and use them, but after 4.0 is released. It might make sense to make the sl driver dynamically expandable at the same time, I guess the tun driver could act as a model for this? > Could you please try the attached patch? Seems to work fine here, and I think it makes sense (though as a continuation of a workaround rather than a proper fix). David. From: "Andrey A. Chernov" To: jkh@freebsd.org, Ruslan Ermilov Cc: dwmalone@maths.tcd.ie, FreeBSD-gnats-submit@FreeBSD.ORG, bde@FreeBSD.ORG Subject: Re: kern/16564: SLIOCSUNIT is broken and can cause panic. Date: Fri, 18 Feb 2000 20:01:37 -0800 Jordan, please approve this patch: On Thu, Feb 17, 2000 at 04:44:57PM +0200, Ruslan Ermilov wrote: > Index: if_sl.c > =================================================================== > RCS file: /usr/FreeBSD-CVS/src/sys/net/if_sl.c,v > retrieving revision 1.70.2.3 > diff -u -p -r1.70.2.3 if_sl.c > --- if_sl.c 1999/12/15 09:17:29 1.70.2.3 > +++ if_sl.c 2000/02/17 14:23:09 > @@ -405,6 +405,7 @@ sltioctl(tp, cmd, data, flag, p) > clist_alloc_cblocks(&tp->t_outq, > SLIP_HIWAT + 2 * sc->sc_if.if_mtu + 1, > SLIP_HIWAT + 2 * sc->sc_if.if_mtu + 1); > + sl_compress_init(&sc->sc_comp, -1); > goto slfound; > } > } -- Andrey A. Chernov http://nagual.pp.ru/~ache/ State-Changed-From-To: open->closed State-Changed-By: ache State-Changed-When: Sun Feb 20 13:04:34 PST 2000 State-Changed-Why: Ruslan's patch applied State-Changed-From-To: closed->feedback State-Changed-By: ru State-Changed-When: Mon Feb 21 01:19:47 PST 2000 State-Changed-Why: This problem affects -stable as well. Responsible-Changed-From-To: freebsd-bugs->ru Responsible-Changed-By: ru Responsible-Changed-When: Mon Feb 21 01:19:47 PST 2000 Responsible-Changed-Why: I will MFC the fix in about a week. State-Changed-From-To: feedback->closed State-Changed-By: ru State-Changed-When: Wed Mar 1 05:39:35 PST 2000 State-Changed-Why: Fix merged into 3.4-stable. >Unformatted: