(review request) ipfw and ipsec processing order for outgoing packets

[Joost Bekkers]

Hi

A while ago there was a discussion about passing packet through pfil before
they are processed by ipsec. I've posted a rough patch back then and I've
finally had time to polish it.

Although the changes seem very invasive at first glance, the .o files created
are identical as long as IPSEC_FILTERGIF is not defined. With FAST_IPSEC diff(1)
will tell you otherwise, but that is due to changed linenumbers which are used
as arguments in two places. I've checked for differences by disassembling (objdump -d)
the .o files.

The attached patch is against 5.3R 

I'm running it myself with FAST_IPSEC. The combination of this patch and the kame
ipsec could do with some more testing.

-- 
greetz Joost
joost at jodocus.org
-------------- next part --------------
*** sys/netinet/dist/ip_output.c	Sat Nov 27 20:56:56 2004
--- sys/netinet/ip_output.c	Mon Nov 29 10:59:48 2004
***************
*** 405,410 ****
--- 405,515 ----
  	}
  
  sendit:
+ 
+ /* Ok, some really weird wizardry going on here. The goal is to pass
+  * packets that are going to be ipsec encapsulated pass through the
+  * firewall first.
+  * 
+  * The PFIL code is split into two pieces:
+  *  1) the main part, which calls the pfil hooks and cleans up after them
+  *  2) a part that handles forwarding (if option IPFIREWALL_FORWARD is in the config)
+  *
+  * Because part 2 is not always include the #defines are done in two steps.
+  *
+  * There are five possible scenarios for this piece of code:
+  *
+  * 1) no ipsec at all			   ->	             pfil
+  * 2) Kame IPSEC without IPSEC_FILTERGIF   ->	      ipsec, pfil
+  * 3) Kame IPSEC with IPSEC_FILTERGIF      ->	pfil, ipsec, pfil
+  * 4) FAST_IPSEC without IPSEC_FILTERGIF   ->	      ipsec, pfil
+  * 5) FAST_IPSEC with IPSEC_FILTERGIF      ->	pfil, ipsec
+  *
+  * The first pfil location is therefor only used if IPSEC_FILTERGIF is defined.
+  * The second location is used if IPSEC_FILTERGIF is not defined of FAST_IPSEC
+  * is not defined.
+  *
+  * The difference in the kame and fast scenarios is caused by fast reinserting
+  * the encapsulated package and 'goto done' where kame will change the current
+  * package to be the encapsulated one. This also causes the strange location of
+  * the first PFIL(); In case of Kame ipsec the code should only be executed if
+  * the packet is actually going to be ipsec-ed. We don't want one packet going
+  * through the firewall twice.
+  */
+ 
+ #define PFIL_RUN_HOOKS(PASSOUT)							\
+ 	/* Jump over all PFIL processing if hooks are not active. */		\
+ 	if (inet_pfil_hook.ph_busy_count == -1)					\
+ 		goto PASSOUT;							\
+ 										\
+ 	/* Run through list of hooks for output packets. */			\
+ 	odst.s_addr = ip->ip_dst.s_addr;					\
+ 	error = pfil_run_hooks(&inet_pfil_hook, &m, ifp, PFIL_OUT, inp);	\
+ 	if (error != 0 || m == NULL)						\
+ 		goto done;							\
+ 										\
+ 	ip = mtod(m, struct ip *);						\
+ 										\
+ 	/* See if destination IP address was changed by packet filter. */	\
+ 	if (odst.s_addr != ip->ip_dst.s_addr) {					\
+ 		m->m_flags |= M_SKIP_FIREWALL;					\
+ 		if (in_localip(ip->ip_dst)) {					\
+ 			m->m_flags |= M_FASTFWD_OURS;				\
+ 			if (m->m_pkthdr.rcvif == NULL)				\
+ 				m->m_pkthdr.rcvif = loif;			\
+ 			if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {		\
+ 				m->m_pkthdr.csum_flags |=			\
+ 				    CSUM_DATA_VALID | CSUM_PSEUDO_HDR;		\
+ 				m->m_pkthdr.csum_data = 0xffff;			\
+ 			}							\
+ 			m->m_pkthdr.csum_flags |=				\
+ 			    CSUM_IP_CHECKED | CSUM_IP_VALID;			\
+ 										\
+ 			error = netisr_queue(NETISR_IP, m);			\
+ 			goto done;						\
+ 		} else								\
+ 			goto again;						\
+ 	}
+ 
+ #define PFIL_FORWARD								\
+ 	/* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */	\
+ 	if (m->m_flags & M_FASTFWD_OURS) {					\
+ 		if (m->m_pkthdr.rcvif == NULL)					\
+ 			m->m_pkthdr.rcvif = loif;				\
+ 		if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {			\
+ 			m->m_pkthdr.csum_flags |=				\
+ 			    CSUM_DATA_VALID | CSUM_PSEUDO_HDR;			\
+ 			m->m_pkthdr.csum_data = 0xffff;				\
+ 		}								\
+ 		m->m_pkthdr.csum_flags |=					\
+ 			    CSUM_IP_CHECKED | CSUM_IP_VALID;			\
+ 										\
+ 		error = netisr_queue(NETISR_IP, m);				\
+ 		goto done;							\
+ 	}									\
+ 	/* Or forward to some other address? */					\
+ 	fwd_tag = m_tag_find(m, PACKET_TAG_IPFORWARD, NULL);			\
+ 	if (fwd_tag) {								\
+ 		if (!in_localip(ip->ip_src) && !in_localaddr(ip->ip_dst)) {	\
+ 			dst = (struct sockaddr_in *)&ro->ro_dst;		\
+ 			bcopy((fwd_tag+1), dst, sizeof(struct sockaddr_in));	\
+ 			m->m_flags |= M_SKIP_FIREWALL;				\
+ 			m_tag_delete(m, fwd_tag);				\
+ 			goto again;						\
+ 		} else {							\
+ 			m_tag_delete(m, fwd_tag);				\
+ 			/* Continue. */						\
+ 		}								\
+ 	}
+ 
+ #ifdef IPFIREWALL_FORWARD
+  #define PFIL(endlabel)	PFIL_RUN_HOOKS(endlabel) \
+ 			PFIL_FORWARD   \
+ 			endlabel:
+ #else
+  #define PFIL(endlabel)	PFIL_RUN_HOOKS(endlabel) \
+ 			endlabel:
+ #endif
+ 
  #ifdef IPSEC
  	/* get SP for this packet */
  	if (inp == NULL)
***************
*** 447,452 ****
--- 552,564 ----
  	default:
  		printf("ip_output: Invalid policy found. %d\n", sp->policy);
  	}
+ #endif
+ 
+ #ifdef IPSEC_FILTERGIF
+   PFIL(passout1)
+ #endif
+ 
+ #ifdef IPSEC
      {
  	struct ipsec_output_state state;
  	bzero(&state, sizeof(state));
***************
*** 654,725 ****
  spd_done:
  #endif /* FAST_IPSEC */
  
! 	/* Jump over all PFIL processing if hooks are not active. */
! 	if (inet_pfil_hook.ph_busy_count == -1)
! 		goto passout;
! 
! 	/* Run through list of hooks for output packets. */
! 	odst.s_addr = ip->ip_dst.s_addr;
! 	error = pfil_run_hooks(&inet_pfil_hook, &m, ifp, PFIL_OUT, inp);
! 	if (error != 0 || m == NULL)
! 		goto done;
! 
! 	ip = mtod(m, struct ip *);
! 
! 	/* See if destination IP address was changed by packet filter. */
! 	if (odst.s_addr != ip->ip_dst.s_addr) {
! 		m->m_flags |= M_SKIP_FIREWALL;
! 		if (in_localip(ip->ip_dst)) {
! 			m->m_flags |= M_FASTFWD_OURS;
! 			if (m->m_pkthdr.rcvif == NULL)
! 				m->m_pkthdr.rcvif = loif;
! 			if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {
! 				m->m_pkthdr.csum_flags |=
! 				    CSUM_DATA_VALID | CSUM_PSEUDO_HDR;
! 				m->m_pkthdr.csum_data = 0xffff;
! 			}
! 			m->m_pkthdr.csum_flags |=
! 			    CSUM_IP_CHECKED | CSUM_IP_VALID;
! 
! 			error = netisr_queue(NETISR_IP, m);
! 			goto done;
! 		} else
! 			goto again;
! 	}
! 
! #ifdef IPFIREWALL_FORWARD
! 	/* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */
! 	if (m->m_flags & M_FASTFWD_OURS) {
! 		if (m->m_pkthdr.rcvif == NULL)
! 			m->m_pkthdr.rcvif = loif;
! 		if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA) {
! 			m->m_pkthdr.csum_flags |=
! 			    CSUM_DATA_VALID | CSUM_PSEUDO_HDR;
! 			m->m_pkthdr.csum_data = 0xffff;
! 		}
! 		m->m_pkthdr.csum_flags |=
! 			    CSUM_IP_CHECKED | CSUM_IP_VALID;
! 
! 		error = netisr_queue(NETISR_IP, m);
! 		goto done;
! 	}
! 	/* Or forward to some other address? */
! 	fwd_tag = m_tag_find(m, PACKET_TAG_IPFORWARD, NULL);
! 	if (fwd_tag) {
! 		if (!in_localip(ip->ip_src) && !in_localaddr(ip->ip_dst)) {
! 			dst = (struct sockaddr_in *)&ro->ro_dst;
! 			bcopy((fwd_tag+1), dst, sizeof(struct sockaddr_in));
! 			m->m_flags |= M_SKIP_FIREWALL;
! 			m_tag_delete(m, fwd_tag);
! 			goto again;
! 		} else {
! 			m_tag_delete(m, fwd_tag);
! 			/* Continue. */
! 		}
! 	}
  #endif
  
- passout:
  	/* 127/8 must not appear on wire - RFC1122. */
  	if ((ntohl(ip->ip_dst.s_addr) >> IN_CLASSA_NSHIFT) == IN_LOOPBACKNET ||
  	    (ntohl(ip->ip_src.s_addr) >> IN_CLASSA_NSHIFT) == IN_LOOPBACKNET) {
--- 766,779 ----
  spd_done:
  #endif /* FAST_IPSEC */
  
! #if !defined FAST_IPSEC || !defined IPSEC_FILTERGIF
!   PFIL(passout2)
  #endif
+ #undef PFIL  
+ #undef PFIL_RUN_HOOKS
+ #undef PFIL_FORWARD
+ 
  
  	/* 127/8 must not appear on wire - RFC1122. */
  	if ((ntohl(ip->ip_dst.s_addr) >> IN_CLASSA_NSHIFT) == IN_LOOPBACKNET ||
  	    (ntohl(ip->ip_src.s_addr) >> IN_CLASSA_NSHIFT) == IN_LOOPBACKNET) {