From owner-svn-src-all@freebsd.org Mon Jul 15 12:27:54 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 83010B8DE2; Mon, 15 Jul 2019 12:27:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 5C7658DEB2; Mon, 15 Jul 2019 12:27:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 9B5E73DC4A5; Mon, 15 Jul 2019 22:27:43 +1000 (AEST) Date: Mon, 15 Jul 2019 22:27:42 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Michael Tuexen cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349989 - head/sys/kern In-Reply-To: <201907142144.x6ELiJr7013093@repo.freebsd.org> Message-ID: <20190715214323.Y1092@besplex.bde.org> References: <201907142144.x6ELiJr7013093@repo.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.2 cv=D+Q3ErZj c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=LS_uVbEeALFgrawW9_YA:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 5C7658DEB2 X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; spf=pass (mx1.freebsd.org: domain of brde@optusnet.com.au designates 211.29.132.42 as permitted sender) smtp.mailfrom=brde@optusnet.com.au X-Spamd-Result: default: False [-5.43 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; RCVD_IN_DNSWL_LOW(-0.10)[42.132.29.211.list.dnswl.org : 127.0.5.1]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; R_SPF_ALLOW(-0.20)[+ip4:211.29.132.0/23]; FREEMAIL_FROM(0.00)[optusnet.com.au]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; DMARC_NA(0.00)[optusnet.com.au]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; IP_SCORE(-2.58)[ip: (-7.38), ipnet: 211.28.0.0/14(-3.19), asn: 4804(-2.35), country: AU(0.01)]; MX_GOOD(-0.01)[cached: extmail.optusnet.com.au]; NEURAL_HAM_SHORT(-0.54)[-0.543,0]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[optusnet.com.au]; ASN(0.00)[asn:4804, ipnet:211.28.0.0/14, country:AU]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_TWO(0.00)[2] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Mon, 15 Jul 2019 12:27:54 -0000 On Sun, 14 Jul 2019, Michael Tuexen wrote: > Log: > Improve the input validation for l_linger. > When using the SOL_SOCKET level socket option SO_LINGER, the structure > struct linger is used as the option value. The component l_linger is of > type int, but internally copied to the field so_linger of the structure > struct socket. The type of so_linger is short, but it is assumed to be > non-negative and the value is used to compute ticks to be stored in a > variable of type int. > > Therefore, perform input validation on l_linger similar to the one > performed by NetBSD and OpenBSD. This still overflows, at least in FreeBSD. FreeBSD really does have type short (or int16_t in the user struct) for l_linger, so overflow now occurs when SHRT_MAX < l.linger <= USHRT_MAX. The overflow is to a negative value on all supported arches. > Thanks to syzkaller for making me aware of this issue. > > Thanks to markj@ for pointing out that a similar check should be added > to so_linger_set(). This check seems to be wrong, but it is hard to be sure since so_linger_set() is never referenced. Using KASSERT() is wrong unless the caller is supposed to have checked the args and usually unnecessary if the caller has checked the args. > Modified: head/sys/kern/uipc_socket.c > ============================================================================== > --- head/sys/kern/uipc_socket.c Sun Jul 14 21:08:54 2019 (r349988) > +++ head/sys/kern/uipc_socket.c Sun Jul 14 21:44:18 2019 (r349989) > @@ -2776,7 +2776,12 @@ sosetopt(struct socket *so, struct sockopt *sopt) > error = sooptcopyin(sopt, &l, sizeof l, sizeof l); > if (error) > goto bad; > - This fixes a style bug (extra blank line separating related code). > + if (l.l_linger < 0 || > + l.l_linger > USHRT_MAX || This adds a style bug (excessive line splitting) together with the wrong limit. The correct limit is INT16_MAX since the user struct uses int16_t and we don't want to allow that to overflow. SHRT_MAX is only the same as INT16_MAX on all supported arches. > + l.l_linger > (INT_MAX / hz)) { This adds a syle bug (excessive parentheses). It is unfortunately technically necessary for a paranoid check. We blindly multiply by hz later and want to ensure no overflow then. Overflow is barely possible with the correct limit of 32767. Then overflow on multiplication by hz would occur with maximal l_linger and 32-bit ints when hz = 65536. hz = 65536 is barely reasonable. > + error = EDOM; > + goto bad; > + } > SOCK_LOCK(so); > so->so_linger = l.l_linger; > if (l.l_onoff) > @@ -4105,6 +4110,9 @@ so_linger_get(const struct socket *so) > void > so_linger_set(struct socket *so, int val) > { > + > + KASSERT(val >= 0 && val <= USHRT_MAX && val <= (INT_MAX / hz), > + ("%s: val %d out of range", __func__, val)); > > so->so_linger = val; > } Unreached code with the same error in the limit and the excessive parentheses, but otherwise fewer style bugs. r180641 in 2008 added mounds of accessor functions like this. Using these is painful compared with direct acccess, and there is currently 1 whole use of so_{state,options,error,linger,protosw}_{set,get} in the sys tree. This is a so_options_get() in cxgbe. cxgbe elsewhere uses 3 direct accesses to so->so_options alone, including 1 '|=' operation which would be especially painful using 2 accessor functions. Bruce