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/
