Bug 16564

Comment 1 ru 2000-02-17 14:44:57 UTC

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

> > 	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.

Comment 3 Andrey A. Chernov 2000-02-19 04:01:37 UTC

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
<ache@nagual.pp.ru>
http://nagual.pp.ru/~ache/

Comment 4 Andrey A. Chernov 2000-02-20 21:04:34 UTC

State Changed
From-To: open->closed

Ruslan's patch applied 

Comment 5 ru 2000-02-21 09:19:47 UTC

State Changed
From-To: closed->feedback

This problem affects -stable as well. 

Comment 6 ru 2000-02-21 09:19:47 UTC

Responsible Changed
From-To: freebsd-bugs->ru

I will MFC the fix in about a week. 

Comment 7 ru 2000-03-01 13:39:35 UTC

State Changed
From-To: feedback->closed

Fix merged into 3.4-stable.