[PATCH] futex: use fault_in to avoid infinite loop

[PATCH] futex: use fault_in to avoid infinite loop

[Cheng Jian]

It will cause softlockup(infinite loop) in kernel
space when we use SYS_set_robust_list in futex which
incoming a misaligned address from user space.

It can be triggered by the following demo

	// futex_align.c

	#include <stdio.h>
	#include <linux/futex.h>
	#include <syscall.h>
	#include <unistd.h>
	#include <stdlib.h>

	int main()
	{
		char *p = malloc(128);

		struct robust_list_head *ro1;
		struct robust_list *entry;
		struct robust_list *pending;

		int ret = 0;

		pid_t pid = getpid();

		printf("size = %d, p %p  pid [%d] \n",
			sizeof(struct robust_list_head), p, pid);

		ro1 = p;
		entry = p + 20;
		pending = p + 40;

		ro1->list.next = entry;
		ro1->list_op_pending = pending;

		entry->next = &(ro1->list);

		ro1->futex_offset = 41;

		*((int *)((char *)entry + 41)) = pid;

		printf(" entry + offert [%p] [%d] \n",
			(int *)((char *)entry + 41),
			*((int *)((char *)entry + 41)));
			ret = syscall(SYS_set_robust_list, ro1,
				sizeof(struct robust_list_head));
		printf("ret = [%d]\n", ret);

		return 0;
	}

It is because LDXER instructions requires the address
which is aligned under arm64 architecture. otherwise
it can trigger an exception, cmpxchg_futex_value_locked
return -EFAULT.

	int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
	{
	retry:
		//......

		/* return -EFAULT */
        	if (cmpxchg_futex_value_locked (& nval, uaddr, uval, mval)) {
			/* always return 0 */
			if (fault_in_user_writeable(uaddr))
				return -1;	/* never here */
		goto retry; /* then goto retry */

		//......
	}

So

	retry - => goto retry -=> retry -=> goto retry ...

Then dead loop here.

So use fault_in to avoid it, It will not enter the retry label
twice under this branch.

Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
---
 kernel/futex.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 76ed592..bc0b14f 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3327,6 +3327,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
 {
 	u32 uval, uninitialized_var(nval), mval;
+	int fault_in = false;
 
 retry:
 	if (get_user(uval, uaddr))
@@ -3351,11 +3352,15 @@ int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
 		 * access fails we try to fault in the futex with R/W
 		 * verification via get_user_pages. get_user() above
 		 * does not guarantee R/W access. If that fails we
-		 * give up and leave the futex locked.
+		 * give up and leave the futex locked. use fault_in
+		 * infinite loop when other exceptions
 		 */
 		if (cmpxchg_futex_value_locked(&nval, uaddr, uval, mval)) {
-			if (fault_in_user_writeable(uaddr))
+			if (unlikely(fault_in) ||
+				fault_in_user_writeable(uaddr)) {
 				return -1;
+			}
+			fault_in = true;
 			goto retry;
 		}
 		if (nval != uval)
-- 
1.8.3.1

Re: [PATCH] futex: use fault_in to avoid infinite loop

[Peter Zijlstra]

On Wed, Dec 06, 2017 at 10:21:07PM +0800, Cheng Jian wrote:
> It will cause softlockup(infinite loop) in kernel
> space when we use SYS_set_robust_list in futex which
> incoming a misaligned address from user space.

Urgh, we should not allow that in the first place.

See how get_futex_key() does:

  if (unlikely(address % sizeof(u32)))
	return -EINVAL;

That same should also be true for the robust list. Using unaligned
variables is insane.

Re: [PATCH] futex: use fault_in to avoid infinite loop

[Peter Zijlstra]

On Wed, Dec 06, 2017 at 05:04:00PM +0100, Peter Zijlstra wrote:
> On Wed, Dec 06, 2017 at 10:21:07PM +0800, Cheng Jian wrote:
> > It will cause softlockup(infinite loop) in kernel
> > space when we use SYS_set_robust_list in futex which
> > incoming a misaligned address from user space.
> 
> Urgh, we should not allow that in the first place.
> 
> See how get_futex_key() does:
> 
>   if (unlikely(address % sizeof(u32)))
> 	return -EINVAL;
> 
> That same should also be true for the robust list. Using unaligned
> variables is insane.

Something a little like so perhaps..

---
Subject: futex: Sanitize user address in set_robust_list()

Passing in unaligned variables messes up cmpxchg on a whole bunch of
architectures. Also, not respecting the natural alignment of data
structures is pretty dumb to begin with.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/uapi/asm-generic/errno.h | 1 +
 kernel/futex.c                   | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..4cb80d4ac160 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -119,5 +119,6 @@
 #define ERFKILL		132	/* Operation not possible due to RF-kill */
 
 #define EHWPOISON	133	/* Memory page has hardware error */
+#define EMORON		134	/* User did something particularly silly */
 
 #endif
diff --git a/kernel/futex.c b/kernel/futex.c
index 76ed5921117a..e2c1a818f88f 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3262,6 +3262,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
 		size_t, len)
 {
+	unsigned long address = (unsigned long)head;
+
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 	/*
@@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
 	if (unlikely(len != sizeof(*head)))
 		return -EINVAL;
 
+	if (unlikely(address % __alignof__(*head)))
+		return -EMORON;
+
 	current->robust_list = head;
 
 	return 0;

Re: [PATCH] futex: use fault_in to avoid infinite loop

[Darren Hart]

On Wed, Dec 06, 2017 at 10:40:08PM +0100, Peter Zijlstra wrote:
> On Wed, Dec 06, 2017 at 05:04:00PM +0100, Peter Zijlstra wrote:
> > On Wed, Dec 06, 2017 at 10:21:07PM +0800, Cheng Jian wrote:
> > > It will cause softlockup(infinite loop) in kernel
> > > space when we use SYS_set_robust_list in futex which
> > > incoming a misaligned address from user space.
> > 
> > Urgh, we should not allow that in the first place.
> > 
> > See how get_futex_key() does:
> > 
> >   if (unlikely(address % sizeof(u32)))
> > 	return -EINVAL;
> > 
> > That same should also be true for the robust list. Using unaligned
> > variables is insane.
> 
> Something a little like so perhaps..
> 
> ---
> Subject: futex: Sanitize user address in set_robust_list()
> 
> Passing in unaligned variables messes up cmpxchg on a whole bunch of
> architectures. Also, not respecting the natural alignment of data
> structures is pretty dumb to begin with.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/uapi/asm-generic/errno.h | 1 +
>  kernel/futex.c                   | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..4cb80d4ac160 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -119,5 +119,6 @@
>  #define ERFKILL		132	/* Operation not possible due to RF-kill */
>  
>  #define EHWPOISON	133	/* Memory page has hardware error */
> +#define EMORON		134	/* User did something particularly silly */

It's baaa-aaack...

(sadly I suspect -EINVAL would be the consistent approach ;-)

>  
>  #endif
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 76ed5921117a..e2c1a818f88f 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3262,6 +3262,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>  		size_t, len)
>  {
> +	unsigned long address = (unsigned long)head;
> +
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>  	/*
> @@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>  	if (unlikely(len != sizeof(*head)))
>  		return -EINVAL;
>  
> +	if (unlikely(address % __alignof__(*head)))
> +		return -EMORON;

Seeing as how this is performing the test as early as possible, would it make
sense to also catch unaligned uaddr and uaddr2 as early as possible too - in
sys_futex?

Something like:

diff --git a/kernel/futex.c b/kernel/futex.c
index 76ed592..c3ee6c4 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -509,8 +509,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
 	 * The futex address must be "naturally" aligned.
 	 */
 	key->both.offset = address % PAGE_SIZE;
-	if (unlikely((address % sizeof(u32)) != 0))
-		return -EINVAL;
 	address -= key->both.offset;
 
 	if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
@@ -3525,6 +3523,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 	u32 val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
+	/* Only allow for aligned uaddr variables */
+	if (unlikely((unsigned long)uaddr % sizeof(u32) != 0 ||
+		     (unsigned long)uaddr2 % sizeof(u32) != 0))
+		return -EINVAL;
+
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
 		      cmd == FUTEX_WAIT_BITSET ||
 		      cmd == FUTEX_WAIT_REQUEUE_PI)) {

I didn't see a need to do anything of the sort to sys_get_robust_list()

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] futex: use fault_in to avoid infinite loop

[Peter Zijlstra]

On Thu, Dec 07, 2017 at 09:21:36PM -0800, Darren Hart wrote:
> On Wed, Dec 06, 2017 at 10:40:08PM +0100, Peter Zijlstra wrote:
> > diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
> > index cf9c51ac49f9..4cb80d4ac160 100644
> > --- a/include/uapi/asm-generic/errno.h
> > +++ b/include/uapi/asm-generic/errno.h
> > @@ -119,5 +119,6 @@
> >  #define ERFKILL		132	/* Operation not possible due to RF-kill */
> >  
> >  #define EHWPOISON	133	/* Memory page has hardware error */
> > +#define EMORON		134	/* User did something particularly silly */
> 
> It's baaa-aaack...
> 

Had to try... will keep trying, we need this! :-)

> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -3262,6 +3262,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> >  SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
> >  		size_t, len)
> >  {
> > +	unsigned long address = (unsigned long)head;
> > +
> >  	if (!futex_cmpxchg_enabled)
> >  		return -ENOSYS;
> >  	/*
> > @@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
> >  	if (unlikely(len != sizeof(*head)))
> >  		return -EINVAL;
> >  
> > +	if (unlikely(address % __alignof__(*head)))
> > +		return -EMORON;
> 
> Seeing as how this is performing the test as early as possible, would it make
> sense to also catch unaligned uaddr and uaddr2 as early as possible too - in
> sys_futex?

Probably makes it clearer that we have this requirement, yes.

Re: [PATCH] futex: use fault_in to avoid infinite loop

[chengjian (D)]

On 2017/12/7 5:40, Peter Zijlstra wrote:
> @@ -3262,6 +3262,8 @@ static int futex_wait_requeue_pi(u32 __user 
> *uaddr, unsigned int flags,
>   SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>   		size_t, len)
>   {
> +	unsigned long address = (unsigned long)head;
> +
>   	if (!futex_cmpxchg_enabled)
>   		return -ENOSYS;
>   	/*
> @@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>   	if (unlikely(len != sizeof(*head)))
>   		return -EINVAL;
>   
> +	if (unlikely(address % __alignof__(*head)))
> +		return -EMORON;
> +

Yeah, This looks nicer. It solved the problem fundamentally
Also for other architecture, such as arm32 which will also
cause a crash without this PATCH.
If we incoming a misaligned address from user space,
the system call will return directly  with a new errno(EMORON).


BUT

	int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
	{
	retry:
		//......

		/* return -EFAULT */
         	if (cmpxchg_futex_value_locked (& nval, uaddr, uval, mval)) {
			/* always return 0 */
			if (fault_in_user_writeable(uaddr))
				return -1;	/* never here */
		goto retry; /* then goto retry */

		//......
	}

Does it correct here?
if we get other exception here next time, does kernel push himself into 
a new  infinite loop ?


Thanks.

CHENG Jian

[tip:locking/urgent] futex: Sanitize user address in set_robust_list()

[tip-bot for Peter Zijlstra]

Commit-ID:  8f3365e34f7519904d78d9fb6dd9e4bae606b9b5
Gitweb:     https://git.kernel.org/tip/8f3365e34f7519904d78d9fb6dd9e4bae606b9b5
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 6 Dec 2017 22:40:08 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 28 Dec 2017 15:19:12 +0100

futex: Sanitize user address in set_robust_list()

Passing in unaligned variables messes up cmpxchg on a whole bunch of
architectures and causes a in kernel lockup when the robust list is
accessed. Also, not respecting the natural alignment of data structures is
pretty dumb to begin with.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: dvhart@infradead.org
Cc: xiexiuqi@huawei.com
Cc: Cheng Jian <cj.chengjian@huawei.com>
Cc: huawei.libin@huawei.com
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20171206214007.GI3857@worktop
---
 include/uapi/asm-generic/errno.h | 1 +
 kernel/futex.c                   | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51a..e306ee4 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -119,5 +119,6 @@
 #define ERFKILL		132	/* Operation not possible due to RF-kill */
 
 #define EHWPOISON	133	/* Memory page has hardware error */
+#define EMORON		134	/* User did something particularly silly */
 
 #endif
diff --git a/kernel/futex.c b/kernel/futex.c
index 57d0b36..4f471aa 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3262,6 +3262,8 @@ out:
 SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
 		size_t, len)
 {
+	unsigned long address = (unsigned long)head;
+
 	if (!futex_cmpxchg_enabled)
 		return -ENOSYS;
 	/*
@@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
 	if (unlikely(len != sizeof(*head)))
 		return -EINVAL;
 
+	if (unlikely(address % __alignof__(*head)))
+		return -EMORON;
+
 	current->robust_list = head;
 
 	return 0;

Re: [PATCH] futex: use fault_in to avoid infinite loop

[Michael Kerrisk]

Peter,

On Wed, Dec 6, 2017 at 10:40 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Dec 06, 2017 at 05:04:00PM +0100, Peter Zijlstra wrote:
>> On Wed, Dec 06, 2017 at 10:21:07PM +0800, Cheng Jian wrote:
>> > It will cause softlockup(infinite loop) in kernel
>> > space when we use SYS_set_robust_list in futex which
>> > incoming a misaligned address from user space.
>>
>> Urgh, we should not allow that in the first place.
>>
>> See how get_futex_key() does:
>>
>>   if (unlikely(address % sizeof(u32)))
>>       return -EINVAL;
>>
>> That same should also be true for the robust list. Using unaligned
>> variables is insane.
>
> Something a little like so perhaps..
>
> ---
> Subject: futex: Sanitize user address in set_robust_list()
>
> Passing in unaligned variables messes up cmpxchg on a whole bunch of
> architectures. Also, not respecting the natural alignment of data
> structures is pretty dumb to begin with.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/uapi/asm-generic/errno.h | 1 +
>  kernel/futex.c                   | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..4cb80d4ac160 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -119,5 +119,6 @@
>  #define ERFKILL                132     /* Operation not possible due to RF-kill */
>
>  #define EHWPOISON      133     /* Memory page has hardware error */
> +#define EMORON         134     /* User did something particularly silly */
>
>  #endif
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 76ed5921117a..e2c1a818f88f 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3262,6 +3262,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>                 size_t, len)
>  {
> +       unsigned long address = (unsigned long)head;
> +
>         if (!futex_cmpxchg_enabled)
>                 return -ENOSYS;
>         /*
> @@ -3270,6 +3272,9 @@ SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
>         if (unlikely(len != sizeof(*head)))
>                 return -EINVAL;
>
> +       if (unlikely(address % __alignof__(*head)))
> +               return -EMORON;
> +

Do we really need to make these sorts of minor insults to user-space
programmers?

Can we make this -EINVAL, please?  (EINVAL in the standard error for
misaligned on calls such as mmap(), mremap(), clone(), read(),
write(), seccomp(), shmat(), and **other futex() operations**.)

Thanks,

Michael


-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/