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