svn commit: r286028 - head/sys/netinet

[Mark Johnston]

On Wed, Jul 29, 2015 at 06:04:02PM +0000, Ermal Luçi wrote:
> Author: eri
> Date: Wed Jul 29 18:04:01 2015
> New Revision: 286028
> URL: https://svnweb.freebsd.org/changeset/base/286028
> 
> Log:
>   ip_output normalization and fixes
>   
>   ip_output has a big chunk of code used to handle special cases with pfil consumers which also forces a reloop on it.
>   Gather all this code together to make it readable and properly handle the reloop cases.
>   
>   Some of the issues identified:
>   
>   M_IP_NEXTHOP is not handled properly in existing code.
>   route reference leaking is possible with in FIB number change
>   route flags checking is not consistent in the function
>   
>   Differential Revision:	https://reviews.freebsd.org/D3022
>   Reviewed by:	gnn
>   Approved by:	gnn(mentor)
>   MFC after:	4 weeks
> 
> Modified:
>   head/sys/netinet/ip_output.c
> 
> Modified: head/sys/netinet/ip_output.c
> ==============================================================================
> --- head/sys/netinet/ip_output.c	Wed Jul 29 17:59:13 2015	(r286027)
> +++ head/sys/netinet/ip_output.c	Wed Jul 29 18:04:01 2015	(r286028)
> @@ -106,6 +106,94 @@ static void	ip_mloopback
>  extern int in_mcast_loop;
>  extern	struct protosw inetsw[];
>  
> +static inline int
> +ip_output_pfil(struct mbuf *m, struct ifnet *ifp, struct inpcb *inp,
> +	struct sockaddr_in *dst, int *fibnum, int *error)
> +{
> +	struct m_tag *fwd_tag = NULL;
> +	struct in_addr odst;
> +	struct ip *ip;
> +
> +	ip = mtod(m, struct ip *);
> +
> +	/* Run through list of hooks for output packets. */
> +	odst.s_addr = ip->ip_dst.s_addr;
> +	*error = pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, inp);
> +	if ((*error) != 0 || m == NULL)
> +		return 1; /* Finished */
> +

This can result in a use-after-free in ip_output() if a pfil hook
consumes the first mbuf in the chain. This happens for example when ipfw
nat is in use: m_megapullup() copies the input packet into a single
cluster, which is returned above. However, ip_output() will continue to
reference the original mbuf chain.

The patch below fixes the problem for me.

Thanks,
-Mark

diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index 0790777..086a8c9 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -107,18 +107,21 @@ extern int in_mcast_loop;
 extern	struct protosw inetsw[];
 
 static inline int
-ip_output_pfil(struct mbuf *m, struct ifnet *ifp, struct inpcb *inp,
-	struct sockaddr_in *dst, int *fibnum, int *error)
+ip_output_pfil(struct mbuf **mp, struct ifnet *ifp, struct inpcb *inp,
+    struct sockaddr_in *dst, int *fibnum, int *error)
 {
 	struct m_tag *fwd_tag = NULL;
+	struct mbuf *m;
 	struct in_addr odst;
 	struct ip *ip;
 
+	m = *mp;
 	ip = mtod(m, struct ip *);
 
 	/* Run through list of hooks for output packets. */
 	odst.s_addr = ip->ip_dst.s_addr;
-	*error = pfil_run_hooks(&V_inet_pfil_hook, &m, ifp, PFIL_OUT, inp);
+	*error = pfil_run_hooks(&V_inet_pfil_hook, mp, ifp, PFIL_OUT, inp);
+	m = *mp;
 	if ((*error) != 0 || m == NULL)
 		return 1; /* Finished */
 
@@ -552,7 +555,7 @@ sendit:
 
 	/* Jump over all PFIL processing if hooks are not active. */
 	if (PFIL_HOOKED(&V_inet_pfil_hook)) {
-		switch (ip_output_pfil(m, ifp, inp, dst, &fibnum, &error)) {
+		switch (ip_output_pfil(&m, ifp, inp, dst, &fibnum, &error)) {
 		case 1: /* Finished */
 			goto done;