From owner-svn-src-all@FreeBSD.ORG Thu May 8 20:50:40 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 47044301; Thu, 8 May 2014 20:50:40 +0000 (UTC) Received: from mail-n.franken.de (drew.ipv6.franken.de [IPv6:2001:638:a02:a001:20e:cff:fe4a:feaa]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mail-n.franken.de", Issuer "Thawte DV SSL CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CACEE230; Thu, 8 May 2014 20:50:39 +0000 (UTC) Received: from [192.168.1.103] (p508F057E.dip0.t-ipconnect.de [80.143.5.126]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTP id EB7CC1C104988; Thu, 8 May 2014 22:50:36 +0200 (CEST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 7.2 \(1874\)) Subject: Re: svn commit: r265691 - head/sys/netinet From: Michael Tuexen In-Reply-To: <20140508202310.GC50446@FreeBSD.org> Date: Thu, 8 May 2014 22:50:28 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: References: <201405081727.s48HRkiT056077@svn.freebsd.org> <20140508202310.GC50446@FreeBSD.org> To: Gleb Smirnoff X-Mailer: Apple Mail (2.1874) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 May 2014 20:50:40 -0000 On 08 May 2014, at 22:23, Gleb Smirnoff wrote: > On Thu, May 08, 2014 at 05:27:46PM +0000, Michael Tuexen wrote: > M> Author: tuexen > M> Date: Thu May 8 17:27:46 2014 > M> New Revision: 265691 > M> URL: http://svnweb.freebsd.org/changeset/base/265691 > M>=20 > M> Log: > M> For some UDP packets (for example with 200 byte payload) and IP = options, > M> the IP header and the UDP header are not in the same mbuf. > M> Add code to in_delayed_cksum() to deal with this case. > M> =20 > M> MFC after: 3 days > M>=20 > M> Modified: > M> head/sys/netinet/ip_output.c > M>=20 > M> Modified: head/sys/netinet/ip_output.c > M> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > M> --- head/sys/netinet/ip_output.c Thu May 8 17:20:45 2014 = (r265690) > M> +++ head/sys/netinet/ip_output.c Thu May 8 17:27:46 2014 = (r265691) > M> @@ -887,15 +887,23 @@ in_delayed_cksum(struct mbuf *m) > M> csum =3D 0xffff; > M> offset +=3D m->m_pkthdr.csum_data; /* checksum offset */ > M> =20 > M> + /* find the mbuf in the chain where the checksum starts*/ > M> + while ((m !=3D NULL) && (offset >=3D m->m_len)) { > M> + offset -=3D m->m_len; > M> + m =3D m->m_next; > M> + } > M> + if (m =3D=3D NULL) { > M> + /* This should not happen. */ > M> + printf("in_delayed_cksum(): checksum outside mbuf = chain.\n"); > M> + return; > M> + } > M> if (offset + sizeof(u_short) > m->m_len) { > M> - printf("delayed m_pullup, m->len: %d off: %d p: %d\n", > M> - m->m_len, offset, ip->ip_p); > M> /* > M> * XXX > M> - * this shouldn't happen, but if it does, the > M> - * correct behavior may be to insert the checksum > M> - * in the appropriate next mbuf in the chain. > M> + * This should not happen, but if it does, it might make = more > M> + * sense to fix the caller than to add code to split it = here. > M> */ > M> + printf("in_delayed_cksum(): checksum split between = mbufs.\n"); > M> return; > M> } > M> *(u_short *)(m->m_data + offset) =3D csum; >=20 > If "this should not happen" really means that we do not expect this = issue > at all, assuming we are coding correctly, then all these comments and = printfs > should be converted to KASSERTs. I tried to keep the style of the code... However, if the first one occurs, we are setting up a packet and have it = too short to contain the checksum. This would really be bug in our code... = So a KASSERT is fine. A KASSERT for the second case is also fine. The only difference between the above and the KASSERT version is that in case of problems the above just sends packets with wrong checksums and the KASSERT version will panic (when compiled with INVARIANTS) or panic (in case of m =3D=3D NULL) or corrupt a byte. However, I also = prefer the KASSERT version, it will help to get the code right... So I committed a change in http://svnweb.freebsd.org/changeset/base/265713 Thanks for your suggestion! Best regards Michael >=20 > If "this should not happen" means 'such packets shouldn't arrrive from > network", then we need to remove printfs, because having a remotely > triggerable printf(9) is a DDoS vulnerability. >=20 > --=20 > Totus tuus, Glebius. >=20