* Thomas Gleixner tglx@linutronix.de wrote:

On Mon, 21 Jan 2019, tip-bot for Kangjie Lu wrote:

...

Commit-ID: 120e4e76857ddbc9268e1aa3f9de61a498e84618 Gitweb: https://git.kernel.org/tip/120e4e76857ddbc9268e1aa3f9de61a498e84618 Author: Kangjie Lu kjlu@umn.edu AuthorDate: Wed, 9 Jan 2019 01:45:24 -0600 Committer: Ingo Molnar mingo@kernel.org CommitDate: Mon, 21 Jan 2019 11:26:17 +0100

sched/core: Fix a potential double-fetch bug in sched_copy_attr()

"uattr->size" is copied in from user space and checked. However, it is copied in again after the security check. A malicious user may race to change it. The fix sets uattr->size to be the checked size.

Signed-off-by: Kangjie Lu kjlu@umn.edu Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Peter Zijlstra peterz@infradead.org Cc: Thomas Gleixner tglx@linutronix.de Cc: pakki001@umn.edu Cc: stable@vger.kernel.org Link: https://lkml.kernel.org/r/20190109074524.10176-1-kjlu@umn.edu Signed-off-by: Ingo Molnar mingo@kernel.org

---

kernel/sched/core.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c index a674c7db2f29..d4d3514c4fe9 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4499,6 +4499,9 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a if (ret) return -EFAULT;

• /* In case attr->size was changed by user-space: */
• attr->size = size;

Just when pondering to send that to Linus, I tried to write up a concise summary for this which made me look at the patch.

If the size changed, then its clear that user space fiddled with the date between the size fetch and the full copy from user. So why restoring the size instead of doing the obvious:

 if (attr->size != size)
	return -ECRAP;

Hmm?

Yeah, indeed - and that's a much more reliable interface behavior in any case. It's probably also faster.

Thanks,

Ingo

On Mon, 28 Jan 2019, Peter Zijlstra wrote:

On Sun, Jan 27, 2019 at 12:04:44PM +0100, Thomas Gleixner wrote:

...

On Mon, 21 Jan 2019, tip-bot for Kangjie Lu wrote:

...

diff --git a/kernel/sched/core.c b/kernel/sched/core.c index a674c7db2f29..d4d3514c4fe9 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4499,6 +4499,9 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a if (ret) return -EFAULT;

• /* In case attr->size was changed by user-space: */
• attr->size = size;

Just when pondering to send that to Linus, I tried to write up a concise summary for this which made me look at the patch.

If the size changed, then its clear that user space fiddled with the date between the size fetch and the full copy from user. So why restoring the size instead of doing the obvious:

 if (attr->size != size)
	return -ECRAP;

Hmm?

Sure; but if we do that we should also change perf_copy_attr() which has the exact same thing.

Yes please.

The point is that by default the data passed to a function (and it does not matter whether it's a syscall) by pointer is immutable. There is exactly ONE syscall which is specifically designed to deal with mutable data and that's a constant source of headache ....

Kangjie is right that all double fetch operations like the one in sched_copy_attr() are prone to concurrent modification problems. But then we really should say NO instead of silently trying to fix things up. I personally would even kill the process immediately, no matter whether the corruption is caused by malicious intent or by sheer stupidity.

Thanks,

tglx