Date: Mon, 15 Jul 2019 22:27:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Michael Tuexen <tuexen@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349989 - head/sys/kern Message-ID: <20190715214323.Y1092@besplex.bde.org> In-Reply-To: <201907142144.x6ELiJr7013093@repo.freebsd.org> References: <201907142144.x6ELiJr7013093@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190715214323.Y1092>