Peter,
On Wed, Dec 6, 2017 at 10:40 PM, Peter Zijlstra <pet...@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) <pet...@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/
