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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
>  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/