Re: cvs commit: src/sys/netinet raw_ip.c

[Aggelos Economopoulos <aoiko@xxxxxxxxxxxxxx>]

On Tuesday 01 July 2008, Hasso Tepper wrote:
> hasso       2008/07/01 01:21:25 PDT
> 
> DragonFly src repository
> 
>   Modified files:
>     sys/netinet          raw_ip.c 
>   Log:
>   Fix [gs]etsockopt(IP_HDRINCL) which allows mere mortals like me to obtain
>   IP addresses via DHCP again.

Great. Only, it turns out the whole approach is flawed. Setting sopt_td to
NULL may help with fooling sooptcopy{in,out} but some code uses the field
to do permission checks. The obvious fix is to add ->sopt_flags and SOPTF_KVA
and make sure no code in the tree leaves the new field uninitialized (lecture
on why open-coding stuff is BAD ommitted). This means that soopt_{from,to}_*
can go. This patch should do the trick; unless testing reveals some issue
I'm going to revert my changes from HEAD and put it in ASAP so it can get
wider testing before the release.

Sorry for the inconvenience,
Aggelos

commit 336fe0988cf8ef2ca8bfeba4dd889ab1e6a92d01
Author: Aggelos Economopoulos <aoiko@cc.ece.ntua.gr>
Date:   Sat Jul 5 13:33:32 2008 +0300

    introduce SOPTF_KVA and use it to avoid copy{in,out} from protocol threads

diff --git a/sys/emulation/linux/linux_socket.c b/sys/emulation/linux/linux_socket.c
index 1ed6717..a801651 100644
--- a/sys/emulation/linux/linux_socket.c
+++ b/sys/emulation/linux/linux_socket.c
@@ -301,6 +301,7 @@ linux_socket(struct linux_socket_args *args, int *res)
 		sopt.sopt_val = &optval;
 		sopt.sopt_valsize = sizeof(optval);
 		sopt.sopt_td = NULL;
+		sopt.sopt_flags = SOPTF_KVA;
 
 		/* We ignore any error returned by setsockopt() */
 		kern_setsockopt(*res, &sopt);
@@ -671,6 +672,7 @@ linux_sendto(struct linux_sendto_args *args, int *res)
 	sopt.sopt_val = &optval;
 	sopt.sopt_valsize = sizeof(optval);
 	sopt.sopt_td = NULL;
+	sopt.sopt_flags = SOPTF_KVA;
 
 	if (kern_getsockopt(linux_args.s, &sopt) != 0)
 		optval = 0;
@@ -1089,6 +1091,7 @@ linux_setsockopt(struct linux_setsockopt_args *args, int *res)
 	sopt.sopt_val = linux_args.optval;
 	sopt.sopt_valsize = linux_args.optlen;
 	sopt.sopt_td = td;
+	sopt.sopt_flags = 0;
 
 	error = kern_setsockopt(linux_args.s, &sopt);
 	return(error);
@@ -1149,6 +1152,7 @@ linux_getsockopt(struct linux_getsockopt_args *args, int *res)
 	sopt.sopt_val = linux_args.optval;
 	sopt.sopt_valsize = valsize;
 	sopt.sopt_td = td;
+	sopt.sopt_flags = 0;
 
 	error = kern_getsockopt(linux_args.s, &sopt);
 	if (error == 0) {
diff --git a/sys/kern/uipc_msg.c b/sys/kern/uipc_msg.c
index fde4c93..3b36a41 100644
--- a/sys/kern/uipc_msg.c
+++ b/sys/kern/uipc_msg.c
@@ -30,11 +30,12 @@
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- * $DragonFly: src/sys/kern/uipc_msg.c,v 1.20 2007/12/19 11:00:22 sephe Exp $
+ * $DragonFly: src/sys/kern/uipc_msg.c,v 1.21 2008/06/17 20:50:11 aggelos Exp $
  */
 
 #include <sys/param.h>
 #include <sys/systm.h>
+#include <sys/kernel.h>
 #include <sys/msgport.h>
 #include <sys/protosw.h>
 #include <sys/socket.h>
@@ -380,23 +381,57 @@ so_pru_sopoll(struct socket *so, int events, struct ucred *cred)
 }
 
 int
-so_pr_ctloutput(struct socket *so, struct sockopt *sopt)
+so_pru_ctloutput(struct socket *so, struct sockopt *sopt)
 {
-	return ((*so->so_proto->pr_ctloutput)(so, sopt));
-#ifdef gag	/* does copyin and copyout deep inside stack XXX JH */
-	struct netmsg_pr_ctloutput msg;
+	struct netmsg_pru_ctloutput msg;
 	lwkt_port_t port;
 	int error;
+	void *uval = NULL;
+	int need_copy;
 
-	port = so->so_proto->pr_mport(so, NULL);
+	/*
+	 * If any of the bits other than SOPTF_KVA are set, we're getting
+	 * garbage from somewhere.
+	 */
+	KKASSERT((sopt->sopt_flags & (~SOPTF_KVA))== 0);
+
+	need_copy = sopt->sopt_td != NULL;
+	if (need_copy) {
+		uval = sopt->sopt_val;
+		/*
+		 * we keep duplicate copies, but for option {s,g}etting
+		 * who cares?
+		 */
+		sopt->sopt_val = kmalloc(sopt->sopt_valsize, M_TEMP, M_WAITOK);
+		error = copyin(uval, sopt->sopt_val, sopt->sopt_valsize);
+		if (error)
+			goto out;
+		sopt->sopt_flags |= SOPTF_KVA;
+	}
+	port = so->so_proto->pr_mport(so, NULL, NULL, PRU_CTLOUTPUT);
 	netmsg_init(&msg.nm_netmsg, &curthread->td_msgport, 0,
 		    netmsg_pru_ctloutput);
-	msg.nm_prfn = so->so_proto->pr_ctloutput;
+	/* TBD: move pr_ctloutput to pr_usrreqs */
+	msg.nm_prufn = so->so_proto->pr_ctloutput;
 	msg.nm_so = so;
 	msg.nm_sopt = sopt;
 	error = lwkt_domsg(port, &msg.nm_netmsg.nm_lmsg, 0);
+out:
+	if (need_copy) {
+		if (!error) {
+			error = copyout(sopt->sopt_val, uval,
+					sopt->sopt_valsize);
+		}
+		/*
+		 * watch out: this may not be the same memory we allocated,
+		 * callees "know" we're using M_TEMP so any reallocations
+		 * will happen from there
+		 */
+		kfree(sopt->sopt_val, M_TEMP);
+		sopt->sopt_val = uval;
+		sopt->sopt_flags &= ~SOPTF_KVA;
+	}
 	return (error);
-#endif
 }
 
 /*
@@ -564,11 +599,11 @@ netmsg_pru_sopoll(netmsg_t msg)
 }
 
 void
-netmsg_pr_ctloutput(netmsg_t msg)
+netmsg_pru_ctloutput(netmsg_t msg)
 {
-	struct netmsg_pr_ctloutput *nm = (void *)msg;
+	struct netmsg_pru_ctloutput *nm = (void *)msg;
 
-	lwkt_replymsg(&msg->nm_lmsg, nm->nm_prfn(nm->nm_so, nm->nm_sopt));
+	lwkt_replymsg(&msg->nm_lmsg, nm->nm_prufn(nm->nm_so, nm->nm_sopt));
 }
 
 void
diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
index f9b1afe..4a141e6 100644
--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -1249,7 +1249,7 @@ sooptcopyin(struct sockopt *sopt, void *buf, size_t len, size_t minlen)
 	if (valsize > len)
 		sopt->sopt_valsize = valsize = len;
 
-	if (sopt->sopt_td != NULL)
+	if ((sopt->sopt_td != NULL) && !(sopt->sopt_flags & SOPTF_KVA))
 		return (copyin(sopt->sopt_val, buf, valsize));
 
 	bcopy(sopt->sopt_val, buf, valsize);
@@ -1268,7 +1268,7 @@ sosetopt(struct socket *so, struct sockopt *sopt)
 	sopt->sopt_dir = SOPT_SET;
 	if (sopt->sopt_level != SOL_SOCKET) {
 		if (so->so_proto && so->so_proto->pr_ctloutput) {
-			return (so_pr_ctloutput(so, sopt));
+			return (so_pru_ctloutput(so, sopt));
 		}
 		error = ENOPROTOOPT;
 	} else {
@@ -1395,7 +1395,7 @@ sosetopt(struct socket *so, struct sockopt *sopt)
 			break;
 		}
 		if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) {
-			(void) so_pr_ctloutput(so, sopt);
+			(void) so_pru_ctloutput(so, sopt);
 		}
 	}
 bad:
@@ -1423,7 +1423,7 @@ sooptcopyout(struct sockopt *sopt, const void *buf, size_t len)
 	valsize = min(len, sopt->sopt_valsize);
 	sopt->sopt_valsize = valsize;
 	if (sopt->sopt_val != 0) {
-		if (sopt->sopt_td != NULL)
+		if ((sopt->sopt_td != NULL) && !(sopt->sopt_flags & SOPTF_KVA))
 			error = copyout(buf, sopt->sopt_val, valsize);
 		else
 			bcopy(buf, sopt->sopt_val, valsize);
@@ -1445,7 +1445,7 @@ sogetopt(struct socket *so, struct sockopt *sopt)
 	sopt->sopt_dir = SOPT_GET;
 	if (sopt->sopt_level != SOL_SOCKET) {
 		if (so->so_proto && so->so_proto->pr_ctloutput) {
-			return (so_pr_ctloutput(so, sopt));
+			return (so_pru_ctloutput(so, sopt));
 		} else
 			return (ENOPROTOOPT);
 	} else {
diff --git a/sys/kern/uipc_syscalls.c b/sys/kern/uipc_syscalls.c
index 1390345..08718b6 100644
--- a/sys/kern/uipc_syscalls.c
+++ b/sys/kern/uipc_syscalls.c
@@ -1069,6 +1069,7 @@ sys_setsockopt(struct setsockopt_args *uap)
 	sopt.sopt_val = uap->val;
 	sopt.sopt_valsize = uap->valsize;
 	sopt.sopt_td = td;
+	sopt.sopt_flags = 0;
 
 	error = kern_setsockopt(uap->s, &sopt);
 	return(error);
@@ -1126,6 +1127,7 @@ sys_getsockopt(struct getsockopt_args *uap)
 	sopt.sopt_val = uap->val;
 	sopt.sopt_valsize = valsize;
 	sopt.sopt_td = td;
+	sopt.sopt_flags = 0;
 
 	error = kern_getsockopt(uap->s, &sopt);
 	if (error == 0) {
diff --git a/sys/net/netisr.h b/sys/net/netisr.h
index 53ad736..89f89ea 100644
--- a/sys/net/netisr.h
+++ b/sys/net/netisr.h
@@ -65,7 +65,7 @@
  *
  *	@(#)netisr.h	8.1 (Berkeley) 6/10/93
  * $FreeBSD: src/sys/net/netisr.h,v 1.21.2.5 2002/02/09 23:02:39 luigi Exp $
- * $DragonFly: src/sys/net/netisr.h,v 1.31 2008/05/02 07:40:32 sephe Exp $
+ * $DragonFly: src/sys/net/netisr.h,v 1.32 2008/06/17 20:50:11 aggelos Exp $
  */
 
 #ifndef _NET_NETISR_H_
@@ -134,13 +134,6 @@ struct netmsg_packet {
     struct mbuf		*nm_packet;
 };
 
-struct netmsg_pr_ctloutput {
-    struct netmsg	nm_netmsg;
-    int			(*nm_prfn) (struct socket *, struct sockopt *);
-    struct socket	*nm_so;
-    struct sockopt	*nm_sopt;
-};
-
 struct netmsg_pr_timeout {
     struct netmsg	nm_netmsg;
     int			(*nm_prfn) (void);
@@ -190,8 +183,8 @@ void netmsg_pru_shutdown(netmsg_t);
 void netmsg_pru_sockaddr(netmsg_t);
 
 void netmsg_pru_sopoll(netmsg_t);
+void netmsg_pru_ctloutput(netmsg_t);
 
-void netmsg_pr_ctloutput(netmsg_t);
 void netmsg_pr_timeout(netmsg_t);
 
 void netmsg_so_notify(netmsg_t);
diff --git a/sys/net/netmsg.h b/sys/net/netmsg.h
index 0b70334..b759a50 100644
--- a/sys/net/netmsg.h
+++ b/sys/net/netmsg.h
@@ -27,7 +27,7 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
- * $DragonFly: src/sys/net/netmsg.h,v 1.6 2007/05/23 08:57:10 dillon Exp $
+ * $DragonFly: src/sys/net/netmsg.h,v 1.7 2008/06/17 20:50:11 aggelos Exp $
  */
 
 #ifndef _NET_NETMSG_H_
@@ -213,4 +213,11 @@ struct netmsg_pru_sopoll {
     struct thread	*nm_td;
 };
 
+struct netmsg_pru_ctloutput {
+    struct netmsg	nm_netmsg;
+    pru_ctloutput_fn_t	nm_prufn;
+    struct socket	*nm_so;
+    struct sockopt	*nm_sopt;
+};
+
 #endif
diff --git a/sys/netgraph/ksocket/ng_ksocket.c b/sys/netgraph/ksocket/ng_ksocket.c
index 334023c..2a27128 100644
--- a/sys/netgraph/ksocket/ng_ksocket.c
+++ b/sys/netgraph/ksocket/ng_ksocket.c
@@ -809,6 +809,7 @@ ng_ksocket_rcvmsg(node_p node, struct ng_mesg *msg,
 			sopt.sopt_level = ksopt->level;
 			sopt.sopt_name = ksopt->name;
 			sopt.sopt_td = NULL;
+			sopt.sopt_flags = SOPTF_KVA;
 			sopt.sopt_valsize = NG_KSOCKET_MAX_OPTLEN;
 			ksopt = (struct ng_ksocket_sockopt *)resp->data;
 			sopt.sopt_val = ksopt->value;
@@ -843,6 +844,7 @@ ng_ksocket_rcvmsg(node_p node, struct ng_mesg *msg,
 			sopt.sopt_val = ksopt->value;
 			sopt.sopt_valsize = valsize;
 			sopt.sopt_td = NULL;
+			sopt.sopt_flags = SOPTF_KVA;
 			error = sosetopt(so, &sopt);
 			break;
 		    }
diff --git a/sys/netproto/smb/smb_trantcp.c b/sys/netproto/smb/smb_trantcp.c
index 3c3e12e..4a26d04 100644
--- a/sys/netproto/smb/smb_trantcp.c
+++ b/sys/netproto/smb/smb_trantcp.c
@@ -91,6 +91,7 @@ nb_setsockopt_int(struct socket *so, int level, int name, int val)
 	sopt.sopt_name = name;
 	sopt.sopt_val = &val;
 	sopt.sopt_valsize = sizeof(val);
+	sopt.sopt_flags |= SOPTF_KVA;
 	return sosetopt(so, &sopt);
 }
 
diff --git a/sys/sys/protosw.h b/sys/sys/protosw.h
index c67a503..750a848 100644
--- a/sys/sys/protosw.h
+++ b/sys/sys/protosw.h
@@ -32,7 +32,7 @@
  *
  *	@(#)protosw.h	8.1 (Berkeley) 6/2/93
  * $FreeBSD: src/sys/sys/protosw.h,v 1.28.2.2 2001/07/03 11:02:01 ume Exp $
- * $DragonFly: src/sys/sys/protosw.h,v 1.21 2008/05/27 01:10:47 dillon Exp $
+ * $DragonFly: src/sys/sys/protosw.h,v 1.22 2008/06/17 20:50:11 aggelos Exp $
  */
 
 #ifndef _SYS_PROTOSW_H_
@@ -167,7 +167,8 @@ struct protosw {
 /* end for protocol's internal use */
 #define PRU_SEND_EOF		23	/* send and close */
 #define	PRU_PRED		24
-#define PRU_NREQ		25
+#define PRU_CTLOUTPUT		25	/* get/set opts */
+#define PRU_NREQ		26
 
 #ifdef PRUREQUESTS
 char *prurequests[] = {
@@ -251,6 +252,7 @@ struct pr_usrreqs {
 				      struct mbuf **controlp, int *flagsp);
 	int	(*pru_sopoll) (struct socket *so, int events,
 				     struct ucred *cred, struct thread *td);
+	int	(*pru_ctloutput) (struct socket *so, struct sockopt *sopt);
 };
 
 typedef int (*pru_abort_fn_t) (struct socket *so);
@@ -290,6 +292,7 @@ typedef int (*pru_soreceive_fn_t) (struct socket *so, struct sockaddr **paddr,
 typedef int (*pru_sopoll_fn_t) (struct socket *so, int events,
 					struct ucred *cred,
 					struct thread *td);
+typedef	int	(*pru_ctloutput_fn_t) (struct socket *so, struct sockopt *sopt);
 
 int	pru_accept_notsupp (struct socket *so, struct sockaddr **nam);
 int	pru_connect_notsupp (struct socket *so, struct sockaddr *nam,
diff --git a/sys/sys/socketops.h b/sys/sys/socketops.h
index 6c0b502..92a1487 100644
--- a/sys/sys/socketops.h
+++ b/sys/sys/socketops.h
@@ -30,7 +30,7 @@
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- * $DragonFly: src/sys/sys/socketops.h,v 1.11 2007/04/22 01:13:17 dillon Exp $
+ * $DragonFly: src/sys/sys/socketops.h,v 1.12 2008/06/17 20:50:11 aggelos Exp $
  */
 
 #ifndef _SOCKETOPS_H_
@@ -94,7 +94,7 @@ int so_pru_sense (struct socket *so, struct stat *sb);
 int so_pru_shutdown (struct socket *so);
 int so_pru_sockaddr (struct socket *so, struct sockaddr **nam);
 int so_pru_sopoll (struct socket *so, int events, struct ucred *cred);
-int so_pr_ctloutput(struct socket *so, struct sockopt *sopt);
+int so_pru_ctloutput(struct socket *so, struct sockopt *sopt);
 
 #endif	/* _KERNEL */
 #endif	/* _SYS_SOCKETOPS_H_ */
diff --git a/sys/sys/socketvar.h b/sys/sys/socketvar.h
index 2a1f84c..c5ba5ef 100644
--- a/sys/sys/socketvar.h
+++ b/sys/sys/socketvar.h
@@ -287,6 +287,10 @@ ssb_space(struct signalsockbuf *ssb)
  * section because it will never be visible to user code.
  */
 enum sopt_dir { SOPT_GET, SOPT_SET };
+enum {
+	SOPTF_KVA = 0x1,	/* ->sopt_val is kernel address */
+};
+
 struct sockopt {
 	enum	sopt_dir sopt_dir; /* is this a get or a set? */
 	int	sopt_level;	/* second arg of [gs]etsockopt */
@@ -294,6 +298,7 @@ struct sockopt {
 	void   *sopt_val;	/* fourth arg of [gs]etsockopt */
 	size_t	sopt_valsize;	/* (almost) fifth arg of [gs]etsockopt */
 	struct	thread *sopt_td; /* calling thread or null if kernel */
+	int	sopt_flags;
 };
 
 struct accept_filter {
diff --git a/sys/vfs/nfs/bootp_subr.c b/sys/vfs/nfs/bootp_subr.c
index 8e50a48..3a76706 100644
--- a/sys/vfs/nfs/bootp_subr.c
+++ b/sys/vfs/nfs/bootp_subr.c
@@ -613,6 +613,7 @@ bootpc_call(struct bootpc_globalcontext *gctx, struct thread *td)
 	sopt.sopt_name = SO_BROADCAST;
 	sopt.sopt_val = &on;
 	sopt.sopt_valsize = sizeof on;
+	sopt.sopt_flags = SOPTF_KVA;
 
 	error = sosetopt(so, &sopt);
 	if (error != 0)
diff --git a/sys/vfs/nfs/krpc_subr.c b/sys/vfs/nfs/krpc_subr.c
index 838cbc9..ff90c82 100644
--- a/sys/vfs/nfs/krpc_subr.c
+++ b/sys/vfs/nfs/krpc_subr.c
@@ -229,6 +229,7 @@ krpc_call(struct sockaddr_in *sa, u_int prog, u_int vers, u_int func,
 	sopt.sopt_name = SO_RCVTIMEO;
 	sopt.sopt_val = &tv;
 	sopt.sopt_valsize = sizeof tv;
+	sopt.sopt_flags |= SOPTF_KVA;
 
 	if ((error = sosetopt(so, &sopt)) != 0)
 		goto out;
diff --git a/sys/vfs/nfs/nfs_socket.c b/sys/vfs/nfs/nfs_socket.c
index f0b09ae..15a2289 100644
--- a/sys/vfs/nfs/nfs_socket.c
+++ b/sys/vfs/nfs/nfs_socket.c
@@ -236,6 +236,7 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
 		sopt.sopt_val = (void *)&ip;
 		sopt.sopt_valsize = sizeof(ip);
 		sopt.sopt_td = NULL;
+		sopt.sopt_flags |= SOPTF_KVA;
 		error = sosetopt(so, &sopt);
 		if (error)
 			goto bad;
@@ -332,6 +333,7 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
 			sopt.sopt_name = SO_KEEPALIVE;
 			sopt.sopt_val = &val;
 			sopt.sopt_valsize = sizeof val;
+			sopt.sopt_flags |= SOPTF_KVA;
 			val = 1;
 			sosetopt(so, &sopt);
 		}
@@ -344,6 +346,7 @@ nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
 			sopt.sopt_name = TCP_NODELAY;
 			sopt.sopt_val = &val;
 			sopt.sopt_valsize = sizeof val;
+			sopt.sopt_flags |= SOPTF_KVA;
 			val = 1;
 			sosetopt(so, &sopt);
 		}
diff --git a/sys/vfs/nfs/nfs_syscalls.c b/sys/vfs/nfs/nfs_syscalls.c
index 1e14b61..d19cf41 100644
--- a/sys/vfs/nfs/nfs_syscalls.c
+++ b/sys/vfs/nfs/nfs_syscalls.c
@@ -379,6 +379,7 @@ nfssvc_addsock(struct file *fp, struct sockaddr *mynam, struct thread *td)
 		sopt.sopt_name = SO_KEEPALIVE;
 		sopt.sopt_val = &val;
 		sopt.sopt_valsize = sizeof val;
+		sopt.sopt_flags |= SOPTF_KVA;
 		val = 1;
 		sosetopt(so, &sopt);
 	}
@@ -392,6 +393,7 @@ nfssvc_addsock(struct file *fp, struct sockaddr *mynam, struct thread *td)
 		sopt.sopt_name = TCP_NODELAY;
 		sopt.sopt_val = &val;
 		sopt.sopt_valsize = sizeof val;
+		sopt.sopt_flags |= SOPTF_KVA;
 		val = 1;
 		sosetopt(so, &sopt);
 	}