On 11/30/2013 03:06 PM, Ingo Molnar wrote: > > * Peter Zijlstra <pet...@infradead.org> wrote: > >> On Thu, Nov 28, 2013 at 12:14:03PM +0100, Juri Lelli wrote: >>> +SYSCALL_DEFINE2(sched_getattr, pid_t, pid, struct sched_attr __user *, >>> attr) >>> { >>> - struct sched_param2 lp; >>> + struct sched_attr lp; >>> struct task_struct *p; >>> int retval; >>> >>> - if (!param2 || pid < 0) >>> + if (!attr || pid < 0) >>> return -EINVAL; >>> >>> + memset(&lp, 0, sizeof(struct sched_attr)); >>> + >>> rcu_read_lock(); >>> p = find_process_by_pid(pid); >>> retval = -ESRCH; >>> @@ -3427,7 +3495,7 @@ SYSCALL_DEFINE2(sched_getparam2, pid_t, pid, struct >>> sched_param2 __user *, param >>> lp.sched_priority = p->rt_priority; >>> rcu_read_unlock(); >>> >>> - retval = copy_to_user(param2, &lp, sizeof(lp)) ? -EFAULT : 0; >>> + retval = copy_to_user(attr, &lp, sizeof(lp)) ? -EFAULT : 0; >>> return retval; >>> >>> out_unlock: >> >> >> So this side needs a bit more care; suppose the kernel has a larger attr >> than userspace knows about. >> >> What would make more sense; add another syscall argument with the >> userspace sizeof(struct sched_attr), or expect userspace to initialize >> attr->size to the right value before calling sched_getattr() ? >> >> To me the extra argument makes more sense; that is: >> >> struct sched_attr attr; >> >> ret = sched_getattr(0, &attr, sizeof(attr)); >> >> seems like a saner thing than: >> >> struct sched_attr attr = { .size = sizeof(attr), }; >> >> ret = sched_getattr(0, &attr); >> >> Mostly because the former has a clear separation between input and >> output arguments, whereas for the second form the attr argument is >> both input and output. >> >> Ingo? > > I suppose so - in the sys_perf_event_open() case we ran out of > arguments, so attr::size was the only sane way to do it. >
Ok, I modified it like this: ------------------------------------------------------------ Subject: [PATCH] fixup: add checks for sys_sched_getattr Add an extra argument to the syscall with the userspace sizeof(struct sched_attr) to be able to handle situations when the kernel has a larger attr than userspace knows about. --- include/linux/syscalls.h | 3 ++- kernel/sched/core.c | 55 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index fbdf44a..45ce599 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -288,7 +288,8 @@ asmlinkage long sys_sched_getscheduler(pid_t pid); asmlinkage long sys_sched_getparam(pid_t pid, struct sched_param __user *param); asmlinkage long sys_sched_getattr(pid_t pid, - struct sched_attr __user *attr); + struct sched_attr __user *attr, + unsigned int size); asmlinkage long sys_sched_setaffinity(pid_t pid, unsigned int len, unsigned long __user *user_mask_ptr); asmlinkage long sys_sched_getaffinity(pid_t pid, unsigned int len, diff --git a/kernel/sched/core.c b/kernel/sched/core.c index fe755f7..b7d91c6 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -3507,7 +3507,7 @@ do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param) } /* - * Mimics kerner/events/core.c perf_copy_attr(). + * Mimics kernel/events/core.c perf_copy_attr(). */ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *attr) @@ -3726,18 +3726,65 @@ out_unlock: return retval; } +static int sched_read_attr(struct sched_attr __user *uattr, + struct sched_attr *attr, + unsigned int size, + unsigned int usize) +{ + int ret; + + if (!access_ok(VERIFY_WRITE, uattr, SCHED_ATTR_SIZE_VER0)) + return -EFAULT; + + /* + * zero the full structure, so that a short copy will be nice. + */ + memset(uattr, 0, sizeof(*uattr)); + + /* + * If we're handed a smaller struct than we know of, + * ensure all the unknown bits are 0 - i.e. old + * user-space does not get uncomplete information. + */ + if (usize < sizeof(*attr)) { + unsigned char *addr; + unsigned char *end; + + addr = (void *)attr + usize; + end = (void *)attr + size; + + for (; addr < end; addr++) + if (*addr) + goto err_size; + } + + ret = copy_to_user(uattr, attr, usize); + if (ret) + return -EFAULT; + +out: + return ret; + +err_size: + ret = -E2BIG; + goto out; +} + /** * sys_sched_getattr - same as above, but with extended "sched_param" * @pid: the pid in question. * @attr: structure containing the extended parameters. + * @size: sizeof(attr) for fwd/bwd comp. */ -SYSCALL_DEFINE2(sched_getattr, pid_t, pid, struct sched_attr __user *, attr) +SYSCALL_DEFINE3(sched_getattr, pid_t, pid, struct sched_attr __user *, attr, + unsigned int, size) { struct sched_attr lp; struct task_struct *p; int retval; - if (!attr || pid < 0) + if (!attr || pid < 0 || size > PAGE_SIZE || + size < SCHED_ATTR_SIZE_VER0) return -EINVAL; memset(&lp, 0, sizeof(struct sched_attr)); @@ -3758,7 +3805,7 @@ SYSCALL_DEFINE2(sched_getattr, pid_t, pid, struct sched_attr __user *, attr) lp.sched_priority = p->rt_priority; rcu_read_unlock(); - retval = copy_to_user(attr, &lp, sizeof(lp)) ? -EFAULT : 0; + retval = sched_read_attr(attr, &lp, sizeof(lp), size); return retval; out_unlock: ----------------------------------------------------------- Do we need to make sched_setattr symmetrical, or, since the user has to fill the fields anyway, we leave it as is? Thanks, - Juri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/