lkml.org 
[lkml]   [2017]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [Y2038] [PATCH v2 08/10] fix get_timespec64() for y2038 safe compat interfaces
    On Fri, Dec 15, 2017 at 4:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
    > On Fri, Dec 15, 2017 at 1:21 AM, Ben Hutchings
    > <ben.hutchings@codethink.co.uk> wrote:
    >> On Mon, 2017-11-27 at 11:30 -0800, Deepa Dinamani wrote:
    >>> get/put_timespec64() interfaces will eventually be used for
    >>> conversions between the new y2038 safe struct __kernel_timespec
    >>> and struct timespec64.
    >>>
    >>> The new y2038 safe syscalls have a common entry for native
    >>> and compat interfaces.
    >>> On compat interfaces, the high order bits of nanoseconds
    >>> should be zeroed out. This is because the application code
    >>> or the libc do not garuntee zeroing of these. If used without
    >>
    >> Spelling: "guarantee"
    >>
    >> [...]
    >>> --- a/kernel/time/time.c
    >>> +++ b/kernel/time/time.c
    >> [...]
    >>> @@ -851,6 +851,11 @@ int get_timespec64(struct timespec64 *ts,
    >>> return -EFAULT;
    >>>
    >>> ts->tv_sec = kts.tv_sec;
    >>> +
    >>> + /* Zero out the padding for 32 bit systems or in compat mode */
    >>> + if (IS_ENABLED(CONFIG_64BIT_TIME) || !IS_ENABLED(CONFIG_64BIT) || in_compat_syscall())
    >>> + kts.tv_nsec &= 0xFFFFFFFFUL;
    >> [...]
    >>
    >> I don't understand the condition here. Suppose we're building for an
    >> architecture that enables the new syscalls and selects ARCH_64BIT_TIME,
    >> but we also enable 64BIT. Then the above condition ends up as:
    >> if (1 || 0 || in_compat_syscall())
    >> so it's always true.
    >>
    >> Should the condition be:
    >> if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall())
    >> or is your intent that architectures only select ARCH_64BIT_TIME if
    >> 64BIT is not enabled?
    >
    > My understanding was that we always enable CONFIG_64BIT_TIME
    > when 64BIT is enabled.
    >
    > For a 64-bit architecture, we must not clear the upper 32 bits of tv_nsec,
    > but instead need later check them for being nonzero. I think the
    > correct condition would be
    >
    > if ((IS_ENABLED(CONFIG_64BIT_TIME) && !IS_ENABLED(CONFIG_64BIT)) ||
    > in_compat_syscall())

    I haven't enabled this by default on all 64 bit architectures.
    The reason I have the condition this way is that I haven't decided how
    I want to handle 64 bit time on x32, and x86 is the first architecture
    I plan to enable this for.
    At that time, this will be reworked based on whatever solution we
    agree on. I did not want to depend on COMPAT_USE_64BIT_TIME yet.

    But, I did mean to do

    if (IS_ENABLED(CONFIG_64BIT_TIME) && (!IS_ENABLED(CONFIG_64BIT)) ||
    in_compat_syscall()))

    I will update this.

    > Two more thoughts:
    >
    > - The temporary variable here is defined as 'struct timespec', this must be
    > changed to __kernel_timespec for the function to work correctly once we
    > switch a 32-bit architecture over. Doing it in this patch is probably the best
    > time for that change.

    Thanks, will do. I will post an update.

    > - I had an idea to handle the copying of timespec/timeval with a
    > one-size-fits-all
    > function and multiple wrappers around it, such as
    >
    > enum user_ts_type {
    > USER_TS_TIMEVAL = 1,
    > USER_TS_32 = 2,
    > USER_TS_CLEARNSEC = 4,
    > USER_TS_NOCHECK = 8,
    > };
    >
    > /* native handlers want to check on 64-bit but zero on 32-bit */
    > #define USER_TS_NATIVE (IS_ENABLED(CONFIG_64BIT) ? 0 : USER_TS_CLEARNSEC)
    >
    > /* compat handlers accessing 64-bit time structs always want to clear
    > the upper half */
    > #define USER_TS_COMPAT64 USER_TS_CLEARNSEC
    >
    > /* on x32, we always use 64-bit time_t but want to clear the upper half */
    > #define USER_TS_COMPAT32 (COMPAT_USE_64BIT_TIME) ? USER_TS_CLEARNSEC :
    > USER_TS_32)
    >
    > int get_timestruct(struct timespec64 *ts, const void __user *uts,
    > enum user_ts_type flags)
    > {
    > int ret;
    >
    > if (flags & USER_TS_32) {
    > struct compat_timespec ts32;
    > ret = copy_from_user(&ts32, uts, sizeof(ts32));
    > if (ret)
    > return -EFAULT;
    > ts->tv_sec = ts32.tv_sec;
    > ts->tv_nsec = ts32.tv_nsec;
    > } else {
    > ret = copy_from_user(&ts, uts, sizeof(*ts));
    > if (ret)
    > return -EFAULT;
    > if (flags & USER_TS_CLEARNSEC)
    > ts->tv_nsec &= 0xFFFFFFFFUL;
    > }
    >
    > if (flags & USER_TS_TIMEVAL) {
    > if (!(flags & USER_TS_NOCHECK) &&
    > ts->tv_nsec >= USEC_PER_SEC)
    > return -EINVAL;
    >
    > ts->tv_nsec *= NSEC_PER_USEC;
    > } else {
    > if (!(flags & USER_TS_NOCHECK) &&
    > ts->tv_nsec >= NSEC_PER_SEC)
    > return -EINVAL;
    > }
    >
    > return 0;
    > }
    >
    > int get_timespec64(struct timespec64 *ts, const struct compat_timespec
    > __user *uts)
    > {
    > return get_timestruct(ts. uts, USER_TS_NATIVE);
    > }
    >
    > int get_compat_timespec32(struct timespec64 *ts, const struct
    > compat_timespec __user *uts)
    > {
    > return get_timestruct(ts. uts, USER_TS_COMPAT32);
    > }
    >
    > int get_compat_timespec64(struct timespec64 *ts, const struct
    > __kernel_timespec __user *uts)
    > {
    > return get_timestruct(ts. uts, USER_TS_COMPAT64);
    > }
    >
    > While working on the driver patches I encountered lots of different
    > combinations of
    > those that might be interesting here, so we could have wrappers for
    > the most common
    > ones and call get_timestruct() and put_timestruct() directly for the less common
    > variations. Am I taking it too far here, or would that make sense?

    I think this is a little confusing.
    nanosleep_copyout() uses a similar strategy. But, I think it makes
    sense in that case as it uses common functions for all versions of
    nanosleep and because of the union.

    -Deepa

    \
     
     \ /
      Last update: 2017-12-18 06:12    [W:3.692 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site