Re: PATCH: fix bug in handling genmask
On Wed, Jan 22, 2014 at 06:29:57AM +0800, Kieran Devlin wrote:
hope this time i get the part ?poke claudio@? right
a. fix a bug.
b. get rid of some junk in ?mask_rnhead?.
c. forbid unprivileged user to insert ?genmask' into ?mask_rnhead'
bug is in this line
memcmp((caddr_t *)info.rti_info[RTAX_GENMASK] + 1, (caddr_t *)t-rn_key
+ 1, ((struct sockaddr *)t-rn_key)-sa_len)
to make this right, at least, it should look like this
memcmp((caddr_t *)info.rti_info[RTAX_GENMASK] + 1, (caddr_t *)t-rn_key
+ 1, ((struct sockaddr *)t-rn_key)-sa_len - 1)
after doing this, the whole checking seems completely unnecessary, is
expected result from ?rn_addmask?.
While your diff is probably right I prefer to just nuke genmask support.
So I propose the following two diffs (kernel, userland) to get rid of it.
I was thinking about genmask a bit and I see no reason to keep it around.
What do people think?
--
:wq Claudio
userland bits
Just remove all genmask / GENMASK references from userland code.
route(8) will no longer allow -genmask but route monitor will still
show GENMASK if something sets it.
route6d no longer needs to look for GENMASK sockaddrs since the kernel
will not allow them.
Index: sbin/route/keywords.h
===
RCS file: /cvs/src/sbin/route/keywords.h,v
retrieving revision 1.27
diff -u -p -r1.27 keywords.h
--- sbin/route/keywords.h 4 Sep 2010 08:06:09 - 1.27
+++ sbin/route/keywords.h 22 Jan 2014 03:05:51 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: keywords.h,v 1.27 2010/09/04 08:06:09 blambert Exp $ */
+/* $OpenBSD$ */
/* WARNING! This file was generated by keywords.sh */
@@ -20,7 +20,6 @@ enum {
K_EXPIRE,
K_FLUSH,
K_GATEWAY,
- K_GENMASK,
K_GET,
K_HOPCOUNT,
K_HOST,
@@ -78,7 +77,6 @@ struct keytab keywords[] = {
{ expire, K_EXPIRE },
{ flush, K_FLUSH },
{ gateway,K_GATEWAY },
- { genmask,K_GENMASK },
{ get,K_GET },
{ hopcount, K_HOPCOUNT },
{ host, K_HOST },
Index: sbin/route/keywords.sh
===
RCS file: /cvs/src/sbin/route/keywords.sh,v
retrieving revision 1.25
diff -u -p -r1.25 keywords.sh
--- sbin/route/keywords.sh 4 Sep 2010 08:06:09 - 1.25
+++ sbin/route/keywords.sh 22 Jan 2014 03:05:40 -
@@ -21,7 +21,6 @@ exec
expire
flush
gateway
-genmask
get
host
hopcount
Index: sbin/route/route.8
===
RCS file: /cvs/src/sbin/route/route.8,v
retrieving revision 1.71
diff -u -p -r1.71 route.8
--- sbin/route/route.8 27 May 2013 14:07:25 - 1.71
+++ sbin/route/route.8 22 Jan 2014 03:06:23 -
@@ -441,15 +441,6 @@ or
modifiers may be used to determine the interface name or interface address.
.Pp
The optional
-.Fl genmask
-modifier specifies that a cloning mask is present.
-This specifies the mask applied when determining if a child route should
-be created.
-It is only applicable to network routes with the
-.Dv RTF_CLONING
-flag set.
-.Pp
-The optional
.Fl label
modifier specifies on route addition or modification that the route
should have the given
Index: sbin/route/route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.165
diff -u -p -r1.165 route.c
--- sbin/route/route.c 28 Oct 2013 15:05:35 - 1.165
+++ sbin/route/route.c 22 Jan 2014 03:07:15 -
@@ -62,7 +62,7 @@
const struct if_status_description
if_status_descriptions[] = LINK_STATE_DESCRIPTIONS;
-union sockunion so_dst, so_gate, so_mask, so_genmask, so_ifa, so_ifp,
so_label, so_src;
+union sockunion so_dst, so_gate, so_mask, so_ifa, so_ifp, so_label, so_src;
typedef union sockunion *sup;
pid_t pid;
@@ -532,11 +532,6 @@ newroute(int argc, char **argv)
usage(1+*argv);
getaddr(RTA_IFP, *++argv, NULL);
break;
- case K_GENMASK:
- if (!--argc)
- usage(1+*argv);
- getaddr(RTA_GENMASK, *++argv, NULL);
- break;
case K_GATEWAY:
if (!--argc)
usage(1+*argv);
@@ -828,9 +823,6 @@ getaddr(int which, char *s, struct hoste
case RTA_NETMASK:
su = so_mask;
break;
- case RTA_GENMASK:
- su = so_genmask;
- break;
case RTA_IFP:
su = so_ifp;
afamily = AF_LINK;
@@ -852,7 +844,6 @@ getaddr(int which, char *s, struct hoste
getaddr(RTA_NETMASK, s, NULL);
Re: PATCH: fix bug in handling genmask
IIRC, I'd talked to both claudio and sthen about removing genmask,
as it's unused and slightly nonsensical. Maybe we should just do
it, if there are actual bugs lurking therein?
On Sat, Dec 28, 2013 at 03:57:04AM +0800, Kieran Devlin wrote:
re-send this patch, loop in claudio@
maybe someone could verify commit this patch
a. fix a bug
b. get rid of junk in ?mask_rnhead'
c. forbid unprivileged user to insert mask into ?mask_rnhead'
bug is in this line
Bcmp((caddr_t *)genmask + 1, (caddr_t *)t-rn_key + 1, ((struct
sockaddr *)t-rn_key)-sa_len)
to make this right, at least, it should look like this
Bcmp((caddr_t)genmask + 1, (caddr_t)t-rn_key + 1, ((struct sockaddr
*)t-rn_key)-sa_len - 1)
after doing this, the whole checking seems completely unnecessary, is
expected result from ?rn_addmask?.
Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.131
diff -p -u -r1.131 rtsock.c
--- net/rtsock.c 1 Nov 2013 20:09:14 - 1.131
+++ net/rtsock.c 27 Dec 2013 14:08:44 -
@@ -593,18 +593,22 @@ route_output(struct mbuf *m, ...)
error = EINVAL;
goto flush;
}
- if (genmask) {
- struct radix_node *t;
- t = rn_addmask(genmask, 0, 1);
- if (t genmask-sa_len =
- ((struct sockaddr *)t-rn_key)-sa_len
- Bcmp((caddr_t *)genmask + 1, (caddr_t *)t-rn_key + 1,
- ((struct sockaddr *)t-rn_key)-sa_len) - 1)
- genmask = (struct sockaddr *)(t-rn_key);
- else {
+ if (genmask rtm-rtm_type != RTM_GET) {
+ if (!(rnh = rt_gettable(dst-sa_family, tableid))) {
+ error = EINVAL;
+ goto flush;
+ }
+ if (!(rn = rn_addmask(genmask, 0, rnh-rnh_treetop-rn_off))) {
error = ENOBUFS;
goto flush;
}
+ if (!((struct sockaddr *)rn-rn_key)-sa_len) {
+ error = EINVAL;
+ goto flush;
+ }
+ genmask = (struct sockaddr *)rn-rn_key;
+ } else {
+ genmask = NULL;
}
#ifdef MPLS
info.rti_mpls = rtm-rtm_mpls;
Re: PATCH: fix bug in handling genmask
maybe, one problem at a time?
first, whether there is actually a bug there, this patch fix it?
and then this removing genmask discussion
as i understand, unless you guys had already reached a conclusion once, there
might be no immediate result
and as blambert@ mentioned, whether it is really a bug might matter
Kieran
On Dec 28, 2013, at 5:19 PM, Bret Lambert blamb...@openbsd.org wrote:
IIRC, I'd talked to both claudio and sthen about removing genmask,
as it's unused and slightly nonsensical. Maybe we should just do
it, if there are actual bugs lurking therein?
On Sat, Dec 28, 2013 at 03:57:04AM +0800, Kieran Devlin wrote:
re-send this patch, loop in claudio@
maybe someone could verify commit this patch
a. fix a bug
b. get rid of junk in ?mask_rnhead'
c. forbid unprivileged user to insert mask into ?mask_rnhead'
bug is in this line
Bcmp((caddr_t *)genmask + 1, (caddr_t *)t-rn_key + 1, ((struct
sockaddr *)t-rn_key)-sa_len)
to make this right, at least, it should look like this
Bcmp((caddr_t)genmask + 1, (caddr_t)t-rn_key + 1, ((struct sockaddr
*)t-rn_key)-sa_len - 1)
after doing this, the whole checking seems completely unnecessary, is
expected result from ?rn_addmask?.
Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.131
diff -p -u -r1.131 rtsock.c
--- net/rtsock.c 1 Nov 2013 20:09:14 - 1.131
+++ net/rtsock.c 27 Dec 2013 14:08:44 -
@@ -593,18 +593,22 @@ route_output(struct mbuf *m, ...)
error = EINVAL;
goto flush;
}
-if (genmask) {
-struct radix_node *t;
-t = rn_addmask(genmask, 0, 1);
-if (t genmask-sa_len =
-((struct sockaddr *)t-rn_key)-sa_len
-Bcmp((caddr_t *)genmask + 1, (caddr_t *)t-rn_key + 1,
-((struct sockaddr *)t-rn_key)-sa_len) - 1)
-genmask = (struct sockaddr *)(t-rn_key);
-else {
+if (genmask rtm-rtm_type != RTM_GET) {
+if (!(rnh = rt_gettable(dst-sa_family, tableid))) {
+error = EINVAL;
+goto flush;
+}
+if (!(rn = rn_addmask(genmask, 0, rnh-rnh_treetop-rn_off))) {
error = ENOBUFS;
goto flush;
}
+if (!((struct sockaddr *)rn-rn_key)-sa_len) {
+error = EINVAL;
+goto flush;
+}
+genmask = (struct sockaddr *)rn-rn_key;
+} else {
+genmask = NULL;
}
#ifdef MPLS
info.rti_mpls = rtm-rtm_mpls;
