Date:   Mon, 2 Jul 2018 10:32:46 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Andy Lutomirski <luto@...capital.net>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-api <linux-api@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Dave Watson <davejwatson@...com>, Paul Turner <pjt@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Russell King <linux@....linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Andi Kleen <andi@...stfloor.org>,
        Chris Lameter <cl@...ux.com>, Ben Maurer <bmaurer@...com>,
        rostedt <rostedt@...dmis.org>,
        Josh Triplett <josh@...htriplett.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Joel Fernandes <joelaf@...gle.com>
Subject: Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are <
 TASK_SIZE

----- On Jun 29, 2018, at 4:39 PM, Andy Lutomirski luto@...capital.net wrote:

> On Fri, Jun 29, 2018 at 12:48 PM, Mathieu Desnoyers
> <mathieu.desnoyers@...icios.com> wrote:
>> There are two aspects I'm concerned about here:
>>
>> 1) security: we don't want 32-bit user-space to feed a 64-bit value over 4GB
>>    as abort_ip that may end up causing OOPSes on architectures that would
>>    lack proper validation of those values on return to userspace.
> 
> I'm not too worried about this.  As long as you're doing it from
> signal-delivery context (which you are AFAICT) you're fine.

No, it's not just signal-delivery context. It's _also_ called from
return to usermode loop, which can by called on return from
interrupt/trap/syscall.

> 
> But I re-read the code and I think I have a really straightforward
> solution.  Two choices:
> 
> (1) Change instruction_pointer_set() to return an error code if the
> address passed in is garbage in a way that could cause unexpected
> behavior (like >=2^32 on x86_64 if regs->cs is 32-bit).  It has very
> very few callers.

This would take care of my security concern wrt abort_ip, but would not
provide consistent behavior for the other fields. Also, perhaps this
kind of change should aim the next merge window ?

> 
> (2) Add instruction_pointer_validate() to go along with
> instruction_pointer_set().
>
> That should be enough to solve the problem, right?

This would only handle the "security" part of the matter, which
is specifically related to rseq->rseq_cs->abort_ip.

What is left is ensuring that we have consistent behavior for
other fields:

[ Note: we have introduced this helper macro: LINUX_FIELD_u32_u64
which defines a field which is 64-bit for 64-bit processes, and 32-bit
with 32-bit of padding for 32-bit processes. ]

* rseq->rseq_cs: (userspace pointer to user-space, updated by user-space
  with single-copy atomicity): current type: LINUX_FIELD_u32_u64,
  cannot be changed to __u64 due to single-copy atomicity requirement,

* rseq->rseq_cs->start_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

* rseq->rseq_cs->post_commit_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

* rseq->rseq_cs->abort_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

For abort_ip, changing the type to __u64 and using the
instruction_pointer_validate() approach you propose would work.

For start_ip and post_commit_ip, we need to decide whether we
want to kill a 32-bit process setting the high bits or if we just
accept and use the full __u64 content on both 32-bit and 64-bit
kernels. Those two fields are only used for arithmetic comparison.
Using the full __u64 content means using 64-bit arithmetic on
32-bit native kernels though.

If we decide to kill the offending process (whether the field type is
__u64 or LINUX_FIELD_u32_u64), we need to be able to figure out if
the process is a compat task when called from signal delivery and from
return to usermode loop (return from irq/trap/syscall).

Comparison with TASK_SIZE solves the issue for abort_ip, post_commit_ip
and start_ip, although nobody seems to like that approach very much.

For rseq->rseq_cs, we cannot use __u64 due to single-copy atomicity
update requirement for 32-bit processes. However, we are using this
field in a copy_from_user(), so it will EFAULT if the high-bits are
set by a compat 32-bit task on a 64-bit kernel. We can therefore check
that the padding is zeroed explicitly on a native 32-bit kernel to
provide a consistent behavior. Specifically because rseq->rseq_cs is
checked with access_ok(), it is therefore enough to check the padding
when __LP64__ is not defined by the preprocessor.

But rather than trying to play games with input validation, I would
favor an approach that would allow rseq to validate all its inputs
straightforwardly. Introducing user_64bit_mode(struct pt_regs *)
across all architectures would allow doing just that. rseq signal
delivery and return to usermode code could then ensure that high bits are
cleared by 32-bit tasks for all fields and thus provide a consistent
behavior for 32-bit tasks running on 32-bit and 64-bit kernels.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists