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 <[email protected]>
---
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
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.
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) <[email protected]>
---
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;
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) <[email protected]>
> ---
> 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
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.
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
Commit-ID: 8f3365e34f7519904d78d9fb6dd9e4bae606b9b5
Gitweb: https://git.kernel.org/tip/8f3365e34f7519904d78d9fb6dd9e4bae606b9b5
Author: Peter Zijlstra <[email protected]>
AuthorDate: Wed, 6 Dec 2017 22:40:08 +0100
Committer: Thomas Gleixner <[email protected]>
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) <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Cheng Jian <[email protected]>
Cc: [email protected]
Cc: [email protected]
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;
Peter,
On Wed, Dec 6, 2017 at 10:40 PM, Peter Zijlstra <[email protected]> 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) <[email protected]>
> ---
> 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/