Re: x86, x32: Correct invalid use of user timespec in the kernel

["H. Peter Anvin" <hpa@zytor.com>]

On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <hpa@linux.intel.com> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
> 
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
> 
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
> 

Hmmm... it ends up being a bit weird even so.  Some of the interfaces
ought to be revamped at a higher level.

Consider this bit in ipc/compat.c:

long compat_sys_semtimedop(int semid, struct sembuf __user *tsems,
                unsigned nsops, const struct compat_timespec __user
*timeout)
{
        struct timespec __user *ts64 = NULL;
        if (timeout) {
                struct timespec ts;
                ts64 = compat_alloc_user_space(sizeof(*ts64));
                if (get_compat_timespec(&ts, timeout))
                        return -EFAULT;
                if (copy_to_user(ts64, &ts, sizeof(ts)))
                        return -EFAULT;
        }
        return sys_semtimedop(semid, tsems, nsops, ts64);
}

This is most definitely broken if COMPAT_USE_64BIT_TIME, even with
get_compat_timespec() is replaced by compat_get_timespec().  However,
what is *really* going on here is that we want to provide a user space
pointer to a kernel-format timespec, so we could have an interface like
this:

int compat_convert_timespec_user(struct compat_timespec **ts64p,
const struct compat_timespec __user *ts)
{
        struct timespec __user *ts64;
        struct timespec ts;

	/* If the compat timespec is 64 bits, no conversion is needed */
	if (!ts || COMPAT_USE_64BIT_TIME) {
		*ts64p = (struct timespec __user *)ts;
		return 0;
	}

	*ts64p = ts64 = compat_alloc_user_space(sizeof(*ts64));
        if (__get_compat_timespec(&ts, timeout))
		return -EFAULT;
	if (copy_to_user(ts64, &ts, sizeof(ts)))
        	return -EFAULT;

	return 0;
}

Now one can argue we have a potential problem with type safety here, but
I'm not sure there is any way to avoid that.

	-hpa