Return to BSD News archive
Newsgroups: comp.unix.bsd
Path: sserve!manuel.anu.edu.au!munnari.oz.au!spool.mu.edu!uunet!news.univie.ac.at!news.tu-graz.ac.at!fstgds01!chmr
From: chmr@fstgds01.tu-graz.ac.at (Christoph Robitschko)
Subject: [386bsd] correct handling of trailers in if_we.c (PATCH)
Message-ID: <1992Dec16.100515.11198@news.tu-graz.ac.at>
Sender: news@news.tu-graz.ac.at (USENET News System)
Nntp-Posting-Host: fstgds01
Organization: Technical University of Graz, Austria
Date: Wed, 16 Dec 92 10:05:15 GMT
Lines: 170
My machine crashed with a type 12 trap when ftping to a local workstation.
This problem was not easy to fix, because there were three bugs:
First, there was a calculation of the start address of the (trailing)
header of a packet, without checking if this address was inside the
ethernet card ring buffer. That caused the length parameter to a bcopy
call to become negative -- crash.
Second, the length of the packet was read from the ethernet card
memory as a short. This did not work, but to assemble it from two
bytes does work (??). I think the compiler was generating bad code.
Anyway, that caused the header offset to become bigger that the length
of the packet, same symptoms as above.
Third, the length of trailer packets was truncated by sizeof(ether_header),
which caused the packet to be dropped.
But finally, I have fixed these problems in the if_we driver. I don't know
if the peoblems exist in other ethernet drivers, if you have used
if_we.c as a porting base, you will want to check.
Christoph
Here's the patch:
*** /sys/i386/isa/if_we.c.patchkit58 Wed Dec 16 10:26:17 1992
--- /sys/i386/isa/if_we.c Wed Dec 16 10:25:18 1992
***************
*** 55,60 ****
--- 55,63 ----
* Allowed interupt handler to look at unit other than 0
* Bdry was static, made it into an array w/ one entry per
* interface. nerd@percival.rain.com (Michael Galassi)
+ *
+ * 15.12.92 - fixed trailer handling in weread() and weget()
+ * chmr@edvz.tu-graz.ac.at (Christoph Robitschko)
*/
#include "we.h"
*************** werint(unit)
*** 572,578 ****
if (len > 30 && len <= ETHERMTU+100
/*&& (*(char *)wer == 1 || *(char *) wer == 0x21)*/)
weread(sc, (caddr_t)(wer + 1), len);
! else printf("reject %d", len);
outofbufs:
wecmd.cs_byte = inb(sc->we_io_nic_addr + WD_P0_COMMAND);
--- 575,581 ----
if (len > 30 && len <= ETHERMTU+100
/*&& (*(char *)wer == 1 || *(char *) wer == 0x21)*/)
weread(sc, (caddr_t)(wer + 1), len);
! else printf("we%d: rejected packet with bogus len %d", unit, len);
outofbufs:
wecmd.cs_byte = inb(sc->we_io_nic_addr + WD_P0_COMMAND);
*************** wesetaddr(physaddr, unit)
*** 709,714 ****
--- 712,718 ----
(((caddr_t)((eh)+1)+(off))) - (sc)->we_vmem_end \
+ (sc)->we_vmem_ring: \
((caddr_t)((eh)+1)+(off)))
+
/*
* Pass a packet to the higher levels.
* We deal with the trailer protocol here.
*************** weread(sc, buf, len)
*** 720,726 ****
{
register struct ether_header *eh;
struct mbuf *m, *weget();
! int off, resid;
/*
* Deal with trailer protocol: if type is trailer type
--- 724,730 ----
{
register struct ether_header *eh;
struct mbuf *m, *weget();
! int off;
/*
* Deal with trailer protocol: if type is trailer type
*************** weread(sc, buf, len)
*** 731,750 ****
eh->ether_type = ntohs((u_short)eh->ether_type);
if (eh->ether_type >= ETHERTYPE_TRAIL &&
eh->ether_type < ETHERTYPE_TRAIL+ETHERTYPE_NTRAILER) {
off = (eh->ether_type - ETHERTYPE_TRAIL) * 512;
if (off >= ETHERMTU) return; /* sanity */
! eh->ether_type = ntohs(*wedataaddr(sc, eh, off, u_short *));
! resid = ntohs(*(wedataaddr(sc, eh, off+2, u_short *)));
if (off + resid > len) return; /* sanity */
len = off + resid;
! } else off = 0;
!
! len -= sizeof(struct ether_header);
if (len <= 0) return;
/*
* Pull packet off interface. Off is nonzero if packet
! * has trailing header; neget will then force this header
* information to be at the front, but we still have to drop
* the type and length which are at the front of any trailer data.
*/
--- 735,761 ----
eh->ether_type = ntohs((u_short)eh->ether_type);
if (eh->ether_type >= ETHERTYPE_TRAIL &&
eh->ether_type < ETHERTYPE_TRAIL+ETHERTYPE_NTRAILER) {
+ caddr_t trailhead;
+ int resid;
+
off = (eh->ether_type - ETHERTYPE_TRAIL) * 512;
if (off >= ETHERMTU) return; /* sanity */
! /* eh->ether_type = ntohs(*wedataaddr(sc, eh, off, u_short *)); */
! /* resid = ntohs(*(wedataaddr(sc, eh, off+2, u_short *))); */
! trailhead = wedataaddr(sc, eh, off, caddr_t);
! eh->ether_type = trailhead[0] * 256 + trailhead[1];
! resid = trailhead[2] * 256 + trailhead[3];
if (off + resid > len) return; /* sanity */
len = off + resid;
! } else {
! off = 0;
! len -= sizeof(struct ether_header);
! }
if (len <= 0) return;
/*
* Pull packet off interface. Off is nonzero if packet
! * has trailing header; weget will then force this header
* information to be at the front, but we still have to drop
* the type and length which are at the front of any trailer data.
*/
*************** weget(buf, totlen, off0, ifp, sc)
*** 828,840 ****
totlen -= len;
/* only do up to end of buffer */
if (cp+len > sc->we_vmem_end) {
! unsigned toend = sc->we_vmem_end - cp;
- bcopy(cp, mtod(m, caddr_t), toend);
- cp = sc->we_vmem_ring;
bcopy(cp, mtod(m, caddr_t)+toend, len - toend);
cp += len - toend;
- epkt = cp + totlen;
} else {
bcopy(cp, mtod(m, caddr_t), (unsigned)len);
cp += len;
--- 839,855 ----
totlen -= len;
/* only do up to end of buffer */
if (cp+len > sc->we_vmem_end) {
! long toend = sc->we_vmem_end - cp;
!
! if (toend > 0)
! bcopy(cp, mtod(m, caddr_t), toend);
! else
! toend = 0;
! cp -= (sc->we_vmem_end - sc->we_vmem_ring);
! epkt -= (sc->we_vmem_end - sc->we_vmem_ring);
bcopy(cp, mtod(m, caddr_t)+toend, len - toend);
cp += len - toend;
} else {
bcopy(cp, mtod(m, caddr_t), (unsigned)len);
cp += len;
--- End of patch ---
--
..signature: Too many levels of symbolic links