From owner-freebsd-net  Wed Oct 11  0:58:46 2000
Delivered-To: freebsd-net@freebsd.org
Received: from whale.sunbay.crimea.ua (whale.sunbay.crimea.ua [212.110.138.65])
	by hub.freebsd.org (Postfix) with ESMTP
	id 7F15037B502; Wed, 11 Oct 2000 00:58:39 -0700 (PDT)
Received: (from ru@localhost)
	by whale.sunbay.crimea.ua (8.11.0/8.11.0) id e9B7wNl59003;
	Wed, 11 Oct 2000 10:58:23 +0300 (EEST)
	(envelope-from ru)
Date: Wed, 11 Oct 2000 10:58:23 +0300
From: Ruslan Ermilov <ru@FreeBSD.org>
To: Archie Cobbs <archie@whistle.com>
Cc: bmilekic@FreeBSD.org, freebsd-net@FreeBSD.org
Subject: Re: ip_input.c patch
Message-ID: <20001011105823.C56373@sunbay.com>
Mail-Followup-To: Archie Cobbs <archie@whistle.com>, bmilekic@FreeBSD.org,
	freebsd-net@FreeBSD.org
References: <200010102202.e9AM2L538821@bubba.whistle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5i
In-Reply-To: <200010102202.e9AM2L538821@bubba.whistle.com>; from archie@whistle.com on Tue, Oct 10, 2000 at 03:02:21PM -0700
Sender: owner-freebsd-net@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.org

On Tue, Oct 10, 2000 at 03:02:21PM -0700, Archie Cobbs wrote:
> Bosko (and anyone else..),
> 
> Does this patch look appropriate to you?
> 
> Thanks,
> -Archie
> 
> ___________________________________________________________________________
> Archie Cobbs   *   Whistle Communications, Inc.  *   http://www.whistle.com
> 
> Index: ip_input.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.141
> diff -u -r1.141 ip_input.c
> --- ip_input.c	2000/09/14 21:06:48	1.141
> +++ ip_input.c	2000/10/10 21:58:46
> @@ -338,15 +338,23 @@
>  		goto bad;
>  	}
>  
> +#if BYTE_ORDER != BIG_ENDIAN
>  	/*
> -	 * Convert fields to host representation.
> +	 * Convert fields to host representation. But first make
> +	 * sure we don't write into a multiply-referenced mbuf.
>  	 */
> +	if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)
> +	    && (m = m_pullup(m, sizeof(*ip))) == NULL) {
> +		ipstat.ips_badhlen++;
> +		return;
> +	}
>  	NTOHS(ip->ip_len);
> +	NTOHS(ip->ip_off);
> +#endif /* !BIG_ENDIAN */
>  	if (ip->ip_len < hlen) {
>  		ipstat.ips_badlen++;
>  		goto bad;
>  	}
> -	NTOHS(ip->ip_off);
>  
>  	/*
>  	 * Check that the amount of data in the buffers

This hunk does not look fine to me.  Firstly, there is no need for
BYTE_ORDER check; endian.h'es already handle this.  Secondly, if you
m_pullup(), `m' may become realloced thus invalidating the `ip' pointer.
And lastly, it does not do anything useful, since at this point `m' is
already pulled up to hlen >= sizeof(struct ip).  Or maybe I am just
overlooking something...

> @@ -599,7 +607,7 @@
>  	 * Reassembly should be able to treat a mbuf cluster, for later
>  	 * operation of contiguous protocol headers on the cluster. (KAME)
>  	 */
> -		if (m->m_flags & M_EXT) {		/* XXX */
> +		if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)) {
>  			if ((m = m_pullup(m, hlen)) == 0) {
>  				ipstat.ips_toosmall++;
>  #ifdef IPFIREWALL_FORWARD
> @@ -688,6 +696,14 @@
>  #ifdef IPDIVERT
>  			/* Restore original checksum before diverting packet */
>  			if (divert_info != 0) {
> +				/* Don't overwrite multiply-referenced mbuf */
> +				if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)
> +				    && (m = m_pullup(m, sizeof(*ip))) == NULL) {
> +#ifdef IPFIREWALL_FORWARD
> +					ip_fw_fwd_addr = NULL;
> +#endif
> +					return;
> +				}
>  				ip->ip_len += hlen;
>  				HTONS(ip->ip_len);
>  				HTONS(ip->ip_off);
The same here.

> @@ -717,6 +733,15 @@
>  		/* Clone packet if we're doing a 'tee' */
>  		if ((divert_info & IP_FW_PORT_TEE_FLAG) != 0)
>  			clone = m_dup(m, M_DONTWAIT);
> +
> +		/* Don't overwrite multiply-referenced mbuf */
> +		if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)
> +		    && (m = m_pullup(m, sizeof(*ip))) == NULL) {
> +#ifdef IPFIREWALL_FORWARD
> +			ip_fw_fwd_addr = NULL;
> +#endif
> +			return;
> +		}
>  
>  		/* Restore packet header fields to original values */
>  		ip->ip_len += hlen;
> 
> 
The same here.


-- 
Ruslan Ermilov		Oracle Developer/DBA,
ru@sunbay.com		Sunbay Software AG,
ru@FreeBSD.org		FreeBSD committer,
+380.652.512.251	Simferopol, Ukraine

http://www.FreeBSD.org	The Power To Serve
http://www.oracle.com	Enabling The Information Age


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-net" in the body of the message