Attached are (very) simple patches which attempt to address the problem.
I've included -net in the CC to solicit a larger audience.
On Sun, 21 Apr 2002, Bill Fenner wrote:
>
> I think that sdl_data should go back to being a variable-length buffer,
> and the source routing stuff should be reimplemented somewhere else
> (perhaps at the end of the variable-length buffer).
>
> What uses the source-routing fields?
>
> Bill
>
Yeah, this is the route I favor, except that it would clearly break
compatibility with 3rd party binary-only drivers. Personally, I would really
like to see a solution implemented in the RELENG_4 branch. To that extent,
the attached patches keep the sockaddr_dl at it's current size but allots the
entire 34 bytes needed for token-ring source routing to the sdl_data field
(for a total of 46 bytes). The token-ring code just embeds it's source
routing information in the sdl_data field now. I also removed setting the
source routing control field to zero for non-iso88025 sockaddr_dl's since all
of the code which examines the field appears to contingent on the interface
being of the iso88025 persuasion.
That said, this leaves ample room in the sockaddr_dl structure for interface
name and MAC address in the sockaddr_dl (too much, but the overall size hasn't
changed). However, token ring interface names are still limited to 6
characters before they risk overflowing the sdl_data field with their source
routing information. This is no worse than the existing situation wherein a
token ring interface with more than 6 characters would cause the last byte(s)
of the hardware address to get clobbered by the source routing control field.
One point I am a little leary of is that in in_arpinput() the original code
appears to have made provision for receiving an ISO88025 frame on a non-token
ring interface and trusted the source routing information contained in such a
frame. First, is this a correct reading of the code? And second, is this
correct behavior? If so, I can easily restore it.
There are 2 sets of attached patches: one for -current and one for -stable
(the one suffixed with a 4). I've tested these pretty extensively on -stable
but haven't done any testing at all for -current (admittingly, not even a
build); furthermore all testing was just with ethernet...I do not have access
to any token ring hardware. I would appreciate any feedback regarding the
approach and anyone who can confirm that I haven't horribly borked token ring
source routing.
If all looks well, then ifconfig (and others?) will have to be updated to
not try and print source routing information unless the interface is token
ring.
Thanks,
Kelly
kbyanc@{posi.net,FreeBSD.org}
The original message for those subscribed to -net but not -arch:
>From [EMAIL PROTECTED] Tue Apr 30 18:13:51 2002
Date: Sun, 21 Apr 2002 01:48:42 -0700 (PDT)
From: Kelly Yancey <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Subject: Overflowing sockaddr_dl's sdl_data buffer
While working on a product at work, I discovered that it is trivial to
overflow the sdl_data buffer in sockaddr_dl structures. In our case, I
enountered the bug by creating a vlan100 interface. The sdl_data buffer is
populated with both the interface name and the parent interface's hardware
address; in his case 7 characters for the interface name and 6 more for the
parent's MAC address for a total of 13 characters (sdl_data is only defined
for 12 characters). As a result, the sdl_rcf field is garbage (actually, the
last octet of the MAC address). While, I worked around the problem in our
product, I would prefer to see the bug fixed in FreeBSD proper.
So, I would like to solicit discussion of the proper fix for this bug.
Should sdl_data's length be extended (say 16 characters)? This would surely
break binary compatibility and only postpones the issue (imagine an interface
with a longer name). Should bound's checking be added to eliminate the
(supposedly optional) interface name from the sdl_data buffer if there is not
room? If so, how does one ensure all drivers (including 3rd party)
perform the bounds-checking? Surely there are other options too. In any
event, the comment in sys/net/if_dl.h for the sdl_data field needs updating
because since the source routing information was added following the sdl_data
field it is impossible for the sdl_data field to be larger than that defined
by the structure definition.
Thanks,
Kelly
kbyanc@{posi.net,FreeBSD.org}
Index: net/if_dl.h
===================================================================
RCS file: /home/cvs/acs/base/src/sys/net/if_dl.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 if_dl.h
--- net/if_dl.h 22 Mar 2002 04:11:00 -0000 1.1.1.1
+++ net/if_dl.h 30 Apr 2002 20:14:09 -0000
@@ -66,10 +66,8 @@
u_char sdl_nlen; /* interface name length, no trailing 0 reqd. */
u_char sdl_alen; /* link level address length */
u_char sdl_slen; /* link layer selector length */
- char sdl_data[12]; /* minimum work area, can be larger;
+ char sdl_data[46]; /* minimum work area, can be larger;
contains both if name and ll address */
- u_short sdl_rcf; /* source routing control */
- u_short sdl_route[16]; /* source routing information */
};
#define LLADDR(s) ((caddr_t)((s)->sdl_data + (s)->sdl_nlen))
Index: net/if_iso88025subr.c
===================================================================
RCS file: /home/cvs/acs/base/src/sys/net/if_iso88025subr.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 if_iso88025subr.c
--- net/if_iso88025subr.c 22 Mar 2002 04:11:00 -0000 1.1.1.1
+++ net/if_iso88025subr.c 30 Apr 2002 21:27:12 -0000
@@ -202,8 +202,8 @@
/* Calculate routing info length based on arp table entry */
if (rt && (sdl = (struct sockaddr_dl *)rt->rt_gateway))
- if (sdl->sdl_rcf != NULL)
- rif_len = TR_RCF_RIFLEN(sdl->sdl_rcf);
+ if (SDL_ISO88025(sdl)->trld_rcf != NULL)
+ rif_len = TR_RCF_RIFLEN(SDL_ISO88025(sdl)->trld_rcf);
/* Generate a generic 802.5 header for the packet */
gen_th.ac = TR_AC;
@@ -212,8 +212,9 @@
if (rif_len) {
gen_th.iso88025_shost[0] |= TR_RII;
if (rif_len > 2) {
- gen_th.rcf = sdl->sdl_rcf;
- memcpy(gen_th.rd, sdl->sdl_route, rif_len - 2);
+ gen_th.rcf = SDL_ISO88025(sdl)->trld_rcf;
+ memcpy(gen_th.rd, SDL_ISO88025(sdl)->trld_route,
+ rif_len - 2);
}
}
Index: net/iso88025.h
===================================================================
RCS file: /home/cvs/acs/base/src/sys/net/iso88025.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 iso88025.h
--- net/iso88025.h 22 Mar 2002 04:11:00 -0000 1.1.1.1
+++ net/iso88025.h 30 Apr 2002 21:27:05 -0000
@@ -102,6 +102,15 @@
u_char fc;
};
+struct iso88025_sockaddr_dl_data {
+ u_short trld_rcf;
+ u_short *trld_route[RIF_MAX_LEN];
+};
+
+#define SDL_ISO88025(s) ((struct iso88025_sockaddr_dl_data *) \
+ ((s)->sdl_data + min((s)->sdl_nlen + \
+ (s)->sdl_alen + (s)->sdl_slen, 12)))
+
/*
* Structure of a 48-bit iso 802.5 address.
* ( We could also add the 16 bit addresses as a union)
Index: netinet/if_ether.c
===================================================================
RCS file: /home/cvs/acs/base/src/sys/netinet/if_ether.c,v
retrieving revision 1.5
diff -u -r1.5 if_ether.c
--- netinet/if_ether.c 29 Mar 2002 20:33:57 -0000 1.5
+++ netinet/if_ether.c 30 Apr 2002 21:27:23 -0000
@@ -529,6 +529,7 @@
register struct arpcom *ac = (struct arpcom *)m->m_pkthdr.rcvif;
struct ether_header *eh;
struct iso88025_header *th = (struct iso88025_header *)0;
+ struct iso88025_sockaddr_dl_data *trld;
register struct llinfo_arp *la = 0;
register struct rtentry *rt;
struct ifaddr *ifa;
@@ -647,7 +648,6 @@
update:
(void)memcpy(LLADDR(sdl), ea->arp_sha, sizeof(ea->arp_sha));
sdl->sdl_alen = sizeof(ea->arp_sha);
- sdl->sdl_rcf = (u_short)0;
/*
* If we receive an arp from a token-ring station over
* a token-ring nic then try to save the source
@@ -655,13 +655,14 @@
*/
if (ac->ac_if.if_type == IFT_ISO88025) {
th = (struct iso88025_header *)m->m_pkthdr.header;
+ trld = SDL_ISO88025(sdl);
rif_len = TR_RCF_RIFLEN(th->rcf);
if ((th->iso88025_shost[0] & TR_RII) &&
(rif_len > 2)) {
- sdl->sdl_rcf = th->rcf;
- sdl->sdl_rcf ^= htons(TR_RCF_DIR);
- memcpy(sdl->sdl_route, th->rd, rif_len - 2);
- sdl->sdl_rcf &= ~htons(TR_RCF_BCST_MASK);
+ trld->trld_rcf = th->rcf;
+ trld->trld_rcf ^= htons(TR_RCF_DIR);
+ memcpy(trld->trld_route, th->rd, rif_len - 2);
+ trld->trld_rcf &= ~htons(TR_RCF_BCST_MASK);
/*
* Set up source routing information for
* reply packet (XXX)
@@ -675,9 +676,7 @@
m->m_data -= 8;
m->m_len += 8;
m->m_pkthdr.len += 8;
- th->rcf = sdl->sdl_rcf;
- } else {
- sdl->sdl_rcf = (u_short)0;
+ th->rcf = trld->trld_rcf;
}
if (rt->rt_expire)
rt->rt_expire = time_second + arpt_keep;
Index: net/if_dl.h
===================================================================
RCS file: /home/ncvs/src/sys/net/if_dl.h,v
retrieving revision 1.11
diff -u -u -r1.11 if_dl.h
--- net/if_dl.h 19 Mar 2002 21:54:16 -0000 1.11
+++ net/if_dl.h 1 May 2002 01:11:42 -0000
@@ -66,10 +66,8 @@
u_char sdl_nlen; /* interface name length, no trailing 0 reqd. */
u_char sdl_alen; /* link level address length */
u_char sdl_slen; /* link layer selector length */
- char sdl_data[12]; /* minimum work area, can be larger;
+ char sdl_data[46]; /* minimum work area, can be larger;
contains both if name and ll address */
- u_short sdl_rcf; /* source routing control */
- u_short sdl_route[16]; /* source routing information */
};
#define LLADDR(s) ((caddr_t)((s)->sdl_data + (s)->sdl_nlen))
Index: net/if_iso88025subr.c
===================================================================
RCS file: /home/ncvs/src/sys/net/if_iso88025subr.c,v
retrieving revision 1.20
diff -u -u -r1.20 if_iso88025subr.c
--- net/if_iso88025subr.c 14 Dec 2001 19:28:43 -0000 1.20
+++ net/if_iso88025subr.c 1 May 2002 01:11:42 -0000
@@ -253,8 +253,8 @@
/* Calculate routing info length based on arp table entry */
if (rt && (sdl = (struct sockaddr_dl *)rt->rt_gateway))
- if (sdl->sdl_rcf != NULL)
- rif_len = TR_RCF_RIFLEN(sdl->sdl_rcf);
+ if (SDL_ISO88025(sdl)->trld_rcf != NULL)
+ rif_len = TR_RCF_RIFLEN(SDL_ISO88025(sdl)->trld_rcf);
/* Generate a generic 802.5 header for the packet */
gen_th.ac = TR_AC;
@@ -264,9 +264,10 @@
if (rif_len) {
gen_th.iso88025_shost[0] |= TR_RII;
if (rif_len > 2) {
- gen_th.rcf = sdl->sdl_rcf;
+ gen_th.rcf = SDL_ISO88025(sdl)->trld_rcf;
(void)memcpy((caddr_t)gen_th.rd,
- (caddr_t)sdl->sdl_route, rif_len - 2);
+ (caddr_t)SDL_ISO88025(sdl)->trld_route,
+ rif_len - 2);
}
}
Index: net/iso88025.h
===================================================================
RCS file: /home/ncvs/src/sys/net/iso88025.h,v
retrieving revision 1.5
diff -u -u -r1.5 iso88025.h
--- net/iso88025.h 18 Mar 2001 05:41:07 -0000 1.5
+++ net/iso88025.h 1 May 2002 01:11:42 -0000
@@ -109,6 +109,15 @@
u_char fc;
};
+struct iso88025_sockaddr_dl_data {
+ u_short trld_rcf;
+ u_short *trld_route[RIF_MAX_LEN];
+};
+
+#define SDL_ISO88025(s) ((struct iso88025_sockaddr_dl_data *) \
+ ((s)->sdl_data + min((s)->sdl_nlen + \
+ (s)->sdl_alen + (s)->sdl_slen, 12)))
+
/*
* Structure of a 48-bit iso 802.5 address.
* ( We could also add the 16 bit addresses as a union)
Index: netinet/if_ether.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.91
diff -u -u -r1.91 if_ether.c
--- netinet/if_ether.c 20 Mar 2002 15:56:36 -0000 1.91
+++ netinet/if_ether.c 1 May 2002 01:11:43 -0000
@@ -571,6 +571,7 @@
struct ether_header *eh;
struct arc_header *arh;
struct iso88025_header *th = (struct iso88025_header *)0;
+ struct iso88025_sockaddr_dl_data *trld;
register struct llinfo_arp *la = 0;
register struct rtentry *rt;
struct ifaddr *ifa;
@@ -697,7 +698,6 @@
}
(void)memcpy(LLADDR(sdl), ar_sha(ah),
sdl->sdl_alen = ah->ar_hln);
- sdl->sdl_rcf = (u_short)0;
/*
* If we receive an arp from a token-ring station over
* a token-ring nic then try to save the source
@@ -705,13 +705,14 @@
*/
if (ifp->if_type == IFT_ISO88025) {
th = (struct iso88025_header *)m->m_pkthdr.header;
+ trld = SDL_ISO88025(sdl);
rif_len = TR_RCF_RIFLEN(th->rcf);
if ((th->iso88025_shost[0] & TR_RII) &&
(rif_len > 2)) {
- sdl->sdl_rcf = th->rcf;
- sdl->sdl_rcf ^= htons(TR_RCF_DIR);
- memcpy(sdl->sdl_route, th->rd, rif_len - 2);
- sdl->sdl_rcf &= ~htons(TR_RCF_BCST_MASK);
+ trld->trld_rcf = th->rcf;
+ trld->trld_rcf ^= htons(TR_RCF_DIR);
+ memcpy(trld->trld_route, th->rd, rif_len - 2);
+ trld->trld_rcf &= ~htons(TR_RCF_BCST_MASK);
/*
* Set up source routing information for
* reply packet (XXX)
@@ -725,9 +726,7 @@
m->m_data -= 8;
m->m_len += 8;
m->m_pkthdr.len += 8;
- th->rcf = sdl->sdl_rcf;
- } else {
- sdl->sdl_rcf = (u_short)0;
+ th->rcf = trld->trld_rcf;
}
if (rt->rt_expire)
rt->rt_expire = time_second + arpt_keep;
