[PATCH] sched: fix a potential double-fetch bug in sched_copy_attr

[PATCH] sched: fix a potential double-fetch bug in sched_copy_attr

[Kangjie Lu]

"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 checks if uattr->size is ever changed after the
check.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 kernel/sched/core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 6fedf3a98581..0a55bdce9a42 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4447,7 +4447,7 @@ do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param)
  */
 static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *attr)
 {
-	u32 size;
+	u32 size, size_cp;
 	int ret;
 
 	if (!access_ok(VERIFY_WRITE, uattr, SCHED_ATTR_SIZE_VER0))
@@ -4460,15 +4460,17 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 	if (ret)
 		return ret;
 
+	size_cp = size;
+
 	/* Bail out on silly large: */
 	if (size > PAGE_SIZE)
 		goto err_size;
 
 	/* ABI compatibility quirk: */
 	if (!size)
-		size = SCHED_ATTR_SIZE_VER0;
+		size_cp = SCHED_ATTR_SIZE_VER0;
 
-	if (size < SCHED_ATTR_SIZE_VER0)
+	else if (size < SCHED_ATTR_SIZE_VER0)
 		goto err_size;
 
 	/*
@@ -4483,7 +4485,7 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 		unsigned char val;
 
 		addr = (void __user *)uattr + sizeof(*attr);
-		end  = (void __user *)uattr + size;
+		end  = (void __user *)uattr + size_cp;
 
 		for (; addr < end; addr++) {
 			ret = get_user(val, addr);
@@ -4492,13 +4494,17 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 			if (val)
 				goto err_size;
 		}
-		size = sizeof(*attr);
+		size_cp = sizeof(*attr);
 	}
 
-	ret = copy_from_user(attr, uattr, size);
+	ret = copy_from_user(attr, uattr, size_cp);
 	if (ret)
 		return -EFAULT;
 
+	/* Sanity check if size was changed in user space */
+	if (attr->size != size)
+		return -EINVAL;
+
 	/*
 	 * XXX: Do we want to be lenient like existing syscalls; or do we want
 	 * to be strict and return an error on out-of-bounds values?
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] sched: fix a potential double-fetch bug in sched_copy_attr

[Peter Zijlstra]

On Tue, Dec 25, 2018 at 04:16:47PM -0600, Kangjie Lu wrote:
> "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 checks if uattr->size is ever changed after the
> check.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---

> +	/* Sanity check if size was changed in user space */
> +	if (attr->size != size)
> +		return -EINVAL;
> +

What perf_copy_attr() does (from whence we copied this code) is:

	attr->size = size;

Would that not also fix things?

[PATCH v2] sched: fix a potential double-fetch bug in sched_copy_attr

[Kangjie Lu]

"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>
---
 kernel/sched/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 6fedf3a98581..e868cc25ac2a 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 in the user space */
+	attr->size = size;
+
 	/*
 	 * XXX: Do we want to be lenient like existing syscalls; or do we want
 	 * to be strict and return an error on out-of-bounds values?
-- 
2.17.1

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

[tip-bot for Kangjie Lu]

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;
+
 	/*
 	 * XXX: Do we want to be lenient like existing syscalls; or do we want
 	 * to be strict and return an error on out-of-bounds values?

Re: [tip:sched/core] sched/core: Fix a potential double-fetch bug in sched_copy_attr()

[Thomas Gleixner]

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?

Thanks,

	tglx

Re: [tip:sched/core] sched/core: Fix a potential double-fetch bug in sched_copy_attr()

[Ingo Molnar]

* 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

Re: [tip:sched/core] sched/core: Fix a potential double-fetch bug in sched_copy_attr()

[Peter Zijlstra]

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.

Re: [tip:sched/core] sched/core: Fix a potential double-fetch bug in sched_copy_attr()

[Thomas Gleixner]

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

Re: [tip:sched/core] sched/core: Fix a potential double-fetch bug in sched_copy_attr()

[Thomas Gleixner]

On Mon, 28 Jan 2019, Thomas Gleixner wrote:
> 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.

Just noticed that this fell through the cracks. Is anyone working on a fix
for this?

Thanks,

	tglx