lkml.org 
[lkml]   [2014]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: x86, x32: Correct invalid use of user timespec in the kernel
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



\
 
 \ /
  Last update: 2014-01-31 20:41    [W:0.078 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site