Date: Mon, 27 Feb 2017 07:56:42 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dmitry Chagin <dchagin@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314293 - head/sys/compat/linux Message-ID: <20170227070338.X992@besplex.bde.org> In-Reply-To: <201702260940.v1Q9egOq029307@repo.freebsd.org> References: <201702260940.v1Q9egOq029307@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 26 Feb 2017, Dmitry Chagin wrote:
> Log:
> Return EOVERFLOW error in case then the size of tv_sec field of struct timespec
> in COMPAT_LINUX32 Linuxulator's not equal to the size of native tv_sec.
This has several bugs, and it is silly to check the range of times (where
overflow can't happen occurs for normal times) while blindly truncating
security-related things like uids (where overflow happens for normal uids).
> Modified: head/sys/compat/linux/linux_time.c
> ==============================================================================
> --- head/sys/compat/linux/linux_time.c Sun Feb 26 09:37:25 2017 (r314292)
> +++ head/sys/compat/linux/linux_time.c Sun Feb 26 09:40:42 2017 (r314293)
> ...
> -void
> +int
> native_to_linux_timespec(struct l_timespec *ltp, struct timespec *ntp)
> {
>
> LIN_SDT_PROBE2(time, native_to_linux_timespec, entry, ltp, ntp);
> -
> +#ifdef COMPAT_LINUX32
> + if (ntp->tv_sec > INT_MAX &&
This doesn't use the parametrization that l_time_t is l_long = int32_t
when COMPAT_LINUX32, but uses an ugly ifdef, then assumes than
int == int32_t.
> + sizeof(ltp->tv_sec) != sizeof(ntp->tv_sec))
> + return (EOVERFLOW);
> +#endif
Negative values are not checked for, so overflow can still occur. Sometimes
negative values overlow to a positive value(e.g., bit pattern
0x8000000000000001 -> 1 in 2's complement. Other times they overflow to
a negative value (e.g., 0x8000000080000001 -> 0x80000001 in 2's complement).
Overflowing values are very unlikely, and the check doesn't prevent them.
E.g., even COMPAT_32 APIs might set a time to 0x7fffffff. The time overflows
to INT32_MIN 1 second later on 32-bit kernels. The kernel shouldn't allow
setting such times, but does. It is left to the user to avoid this foot-
shooting. Since setting critical times is privileged, the user usually
doesn't do this. On 64-bit kernels, setting such times works and no
overflow occurs for native times, but unsuspecting 32-bit applications
are broken, so again it is left to privileged users to avoid this foot-
shooting (now for 32-bit feet).
Similarly for negative times. They are even less useful than times after
32-bit time_t overflows, but in some cases they are not errors so must
not be rejected or blindly truncated by conversion functions.
Overflow of tv_nsec is not checked for. It really can't happen, since
although native time_t is required to be long by historical mistakes,
and long might tbe 64 bits or larger, on values between 0 and 10**9-1
are valid for it, and these are representable by 32-bit long. This
depends on the conversion function only being from native times which
are presumably already active so have been checked for validity.
uids overflow routinely. E.g., the error uid is (uid_t)-1 = 0xffffffff
and blind truncation of this in linux_uid16.c overflows to 0xffff which
is the 16-bit error uid (uid16_t)-1. But 0xffff = 65535 is a valid
native uid. The valid native uid 0x10000 overflows to 0 (root).
The valid native uid 0xfffffffe (nobody for nfs) overflows to 0xfffe
(nobody).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170227070338.X992>