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

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