From owner-svn-src-all@FreeBSD.ORG Sat Nov 2 05:22:23 2013 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 ESMTP id C046EDC9; Sat, 2 Nov 2013 05:22:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 8401D2184; Sat, 2 Nov 2013 05:22:23 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id ABCBFD41E16; Sat, 2 Nov 2013 16:22:13 +1100 (EST) Date: Sat, 2 Nov 2013 16:22:12 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Adrian Chadd Subject: Re: svn commit: r257535 - head/sys/netgraph In-Reply-To: <201311020011.rA20BchL020170@svn.freebsd.org> Message-ID: <20131102151309.A1102@besplex.bde.org> References: <201311020011.rA20BchL020170@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=YYGEuWhf c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=EA1U1QKjvecA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=STxCta6Vch0A:10 a=5QIHI7fnYl3m6JV1XdwA:9 a=oWUZixYzURRpJlRl:21 a=pUqxu9-Ujg5mhHLm:21 a=CjuIK1q_8ugA:10 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.14 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: Sat, 02 Nov 2013 05:22:23 -0000 On Sat, 2 Nov 2013, Adrian Chadd wrote: > Log: > Teach the netgraph code to use a const char * pointer too. This actually fixes a much larger bug that was apparently accidentally exposed by new const poisoning and new smaller type bugs. > Pointy hat to: adrian Outpointed by the previous bug. > Modified: head/sys/netgraph/ng_iface.c > ============================================================================== > --- head/sys/netgraph/ng_iface.c Fri Nov 1 23:30:54 2013 (r257534) > +++ head/sys/netgraph/ng_iface.c Sat Nov 2 00:11:38 2013 (r257535) > @@ -776,7 +776,7 @@ ng_iface_rcvdata(hook_p hook, item_p ite > return (EAFNOSUPPORT); > } > if (harvest.point_to_point) > - random_harvest(&(m->m_data), 12, 2, RANDOM_NET_NG); '&(m->m_data)' is not just a pair of style bugs. It gives address of the pointer (somewhere in the mbuf header), not the address of pointed- to data, so the randomness was almost null. The style bugs are excessive parentheses and chumminess with the implementation (non-use of the accessor function mtod()). > + random_harvest(mtod(m, const void *), 12, 2, RANDOM_NET_NG); Presumably you really do want to harvest the pointed-to data and there are at least 12 bytes of it, so the semantic fix isn't backwards or a buffer overrun. > M_SETFIB(m, ifp->if_fib); > netisr_dispatch(isr, m); > return (0); const poisoning sometimes exposes garbage pointers accidentally. It seems to take a new design error (random_harvest() taking a const char * instead of a const void *) for this bug to be detected. random_harvest() used to take a void * arg and the garbage pointer was automatically converted to that. Conversion to const void * would be just as automatic. The log message says const char * but the code says const void *. The latter is correct, at least mtod() is used, since the mtod() API unfortunately requires naming a type and const void * is the best type to use here. Then futher conversions apparently occur: if there is actually a const char * anywhere, it is wrong, and if we convert this const void * to it then we must convert to const u_char * to actually access the data It is best for intermediate conversions to only go through const void * and not expand the old design error of making them go through caddr_t (for m_data). Bruce