lkml.org 
[lkml]   [2017]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] futex: futex_wake_op, do not fail on invalid op
From
Date
Hi,

is this OK/going to be applied to 4.14 yet or postponed to 4.15-rc?

On 10/23/2017, 01:41 PM, Jiri Slaby wrote:
> In 30d6e0a4190d ("futex: Remove duplicated code and fix undefined
> behaviour"), I let FUTEX_WAKE_OP to fail on invalid op. Namely when
> op should be considered as shift and the shift is out of range (< 0 or
>> 31).
>
> But strace's test suite does this madness:
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xa0caffee);
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xbadfaced);
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xffffffff);
>
> When I pick the first 0xa0caffee, it decodes as:
> 0x80000000 & 0xa0caffee: oparg is shift
> 0x70000000 & 0xa0caffee: op is FUTEX_OP_OR
> 0x0f000000 & 0xa0caffee: cmp is FUTEX_OP_CMP_EQ
> 0x00fff000 & 0xa0caffee: oparg is sign-extended 0xcaf = -849
> 0x00000fff & 0xa0caffee: cmparg is sign-extended 0xfee = -18
>
> That means the op tries to do this:
> (futex |= (1 << (-849))) == -18
> which is completely bogus. The new check of op in the code is:
> if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> if (oparg < 0 || oparg > 31)
> return -EINVAL;
> oparg = 1 << oparg;
> }
>
> which results obviously in the "Invalid argument" errno:
> ----8<--------8<--------8<--------8<--------8<----
> FAIL: futex
> ===========
>
> futex(0x7fabd78bcffc, 0x5, 0xfacefeed, 0xb, 0x7fabd78bcffc, 0xa0caffee) = -1: Invalid argument
> futex.test: failed test: ../futex failed with code 1
> ----8<--------8<--------8<--------8<--------8<----
>
> So let us soften the failure to print only a (ratelimited) message,
> crop the value and continue as if it were right. When userspace keeps
> up, we can switch this to return -EINVAL again.
>
> [v2]
> Do not return 0 immediatelly, proceed with the cropped value.
>
> Fixes: 30d6e0a4190d ("futex: Remove duplicated code and fix undefined behaviour")
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> kernel/futex.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 0518a0bfc746..0d638f008bb1 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1570,8 +1570,16 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> int oldval, ret;
>
> if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> - if (oparg < 0 || oparg > 31)
> - return -EINVAL;
> + if (oparg < 0 || oparg > 31) {
> + char comm[sizeof(current->comm)];
> + /*
> + * kill this print and return -EINVAL when userspace
> + * is sane again
> + */
> + pr_info_ratelimited("futex_wake_op: %s tries to shift op by %d; fix this program\n",
> + get_task_comm(comm, current), oparg);
> + oparg &= 31;
> + }
> oparg = 1 << oparg;
> }
>
>


--
js
suse labs

\
 
 \ /
  Last update: 2017-11-02 07:54    [W:0.147 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site