Messages in this thread | | | Date | Fri, 31 Jan 2014 11:13:57 -0800 | From | "H. Peter Anvin" <> | Subject | Re: 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
| |