Messages in this thread | | | Subject | Re: [PATCH v2 1/2] futex: futex_wake_op, do not fail on invalid op | From | Jiri Slaby <> | Date | Thu, 2 Nov 2017 07:54:14 +0100 |
| |
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
| |